The unique name algorithm is using a counter, on each file collision, the collision resolution will take more time, at the end, it might create a stampede and ultimately bring the system down.
I propose to keep the external behaviour intact but drop the incremental approach.
The patch is trivial and use random seek instead of iteration to find a suitable name.
this will allow O(log(m, n)) complexity and solve a collision in a few seek instead of thousands.
The logic can probably be easily backported.
Comment | File | Size | Author |
---|---|---|---|
#19 | core-file_create_filename-2684403-19.patch | 5.07 KB | xdecock |
#17 | core-file_create_filename-2684403-17.patch | 3.93 KB | dagmar |
#12 | core-file_create_filename-2684403-12-8.1.patch | 3.83 KB | xdecock |
#7 | file_create_filename_2684403_5.diff | 832 bytes | arunkumark |
#5 | file_create_filename_new.patch | 1.27 KB | xdecock |
Comments
Comment #2
dagmarComment #4
cilefen CreditAttribution: cilefen commented@xdecock You will have to instantiate Random before using it like
$random = new Random();
.Utility classes are sometimes Symfony container services, but Random is not one of them.
Comment #5
xdecock CreditAttribution: xdecock commentedHi,
sorry, corrected the patch, indeed should have been more carefull
Comment #6
arunkumarkHi,
I have re patched based on the comment #4
Comment #7
arunkumarkComment #8
cilefen CreditAttribution: cilefen commented@arunkumark Why did you hide the #5 patch? Oh, I see you double-posted your patch.
Comment #9
dagmarthanks @xdecock,
Please consider using a different name for your patch https://www.drupal.org/patch/submit#patch_naming
Drupal coding standards indicates you should use NULL instead of null.
false should be FALSE
Comment #11
cilefen CreditAttribution: cilefen commentedSome of the test failures, which would fail with the patches from #7 or the more optimized #5, are because certain tests are looking for specific integer counters in filenames. As part of this issue, a way will have to be found to adapt the tests.
Comment #12
xdecock CreditAttribution: xdecock commentedHello,
i've corrected the tests, I'll try to deploy a test suite and tackle the todo for multiple copies.
I'm not a drupal dev, more performance troubleshooter and encountered a bunch of meltdown due to this part of code (usually made worse by nfs).
I'll allready submit the existing patch for review
PS : sorry about the coding convention, so on, i searched and found it a bit difficult to land on the page with the convention, but as i said, i'm not really doing a lot of dev anymore
Comment #13
dagmarThanks @xdecock, let see what the bot says about this patch.
Comment #16
cilefen CreditAttribution: cilefen commented@xdecock Don't worry about the coding conventions. You can ask for another volunteer to sort that out if needed. The project appreciates and values all contributions, especially performance-based.
We need individuals with your skill set to do many things, such as testing BigPipe.
Comment #17
dagmarUpdated to 8.2.x.
It seems we have less code coverage now since we when a file is moved there is no information about its new name (at least using the current API). What should be the approach here?
Comment #19
xdecock CreditAttribution: xdecock commentedI'm working on a with fixing the mt_seed which will render the letters "predictable"
It's a bit messy, but at least, it expands code coverage, i'll probably cleanup my install to ensure it's 8.3 ready
and see if i have time to deploy the test db on my computer.
I'll try to do it today
Comment #25
amonteroHappens also on Drupal 7 latest.
Tentatively tagging for backport as soon as it gets into 8.x.