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.

CommentFileSizeAuthor
#19 core-file_create_filename-2684403-19.patch5.07 KBxdecock
#17 core-file_create_filename-2684403-17.patch3.93 KBdagmar
#12 core-file_create_filename-2684403-12-8.1.patch3.83 KBxdecock
#7 file_create_filename_2684403_5.diff832 bytesarunkumark
#6 file_create_filename_2684403_5.diff832 bytesarunkumark
#5 file_create_filename_new.patch1.27 KBxdecock
file_create_filename.patch811 bytesxdecock

Comments

xdecock created an issue. See original summary.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, file_create_filename.patch, failed testing.

cilefen’s picture

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.

xdecock’s picture

xdecock commented
StatusFileSize
new file_create_filename_new.patch1.27 KB

Hi,

sorry, corrected the patch, indeed should have been more carefull

arunkumark’s picture

arunkumark
Location Coimbatore
commented
StatusFileSize
new file_create_filename_2684403_5.diff832 bytes

Hi,
I have re patched based on the comment #4

arunkumark’s picture

arunkumark
Location Coimbatore
commented
Status: Needs work » Needs review
StatusFileSize
new file_create_filename_2684403_5.diff832 bytes
cilefen’s picture

cilefen commented

@arunkumark Why did you hide the #5 patch? Oh, I see you double-posted your patch.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
Status: Needs review » Needs work

thanks @xdecock,

Please consider using a different name for your patch https://www.drupal.org/patch/submit#patch_naming

  1. +++ b/core/includes/file.inc
@@ -690,6 +691,7 @@ function file_unmunge_filename($filename) {
+  static $random = null;

    Drupal coding standards indicates you should use NULL instead of null.

  2. +++ b/core/includes/file.inc
@@ -720,9 +722,14 @@ function file_create_filename($basename, $directory) {
+      $randsuffix .= $random->name(1, false);

    false should be FALSE

The last submitted patch, 7: file_create_filename_2684403_5.diff, failed testing.

cilefen’s picture

cilefen commented

Some 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.

xdecock’s picture

xdecock commented
StatusFileSize
new core-file_create_filename-2684403-12-8.1.patch3.83 KB

Hello,

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

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
Status: Needs work » Needs review

Thanks @xdecock, let see what the bot says about this patch.

The last submitted patch, 5: file_create_filename_new.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: core-file_create_filename-2684403-12-8.1.patch, failed testing.

cilefen’s picture

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.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
StatusFileSize
new core-file_create_filename-2684403-17.patch3.93 KB

Updated 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?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xdecock’s picture

xdecock commented
StatusFileSize
new core-file_create_filename-2684403-19.patch5.07 KB

I'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

Status: Needs review » Needs work

The last submitted patch, 19: core-file_create_filename-2684403-19.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amontero’s picture

amontero
Location Barcelona
commented
Issue tags: +Needs backport to D7

Happens also on Drupal 7 latest.
Tentatively tagging for backport as soon as it gets into 8.x.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

smustgrave’s picture

smustgrave commented
Issue tags: -Needs backport to D7 +Bug Smash Initiative, +Needs issue summary update, +Needs steps to reproduce

This came up as the daily BSI target

To help triage can the summary be updated using the standard issue template with clear steps to reproduce please.

I see the patch has test coverage but would like to review in an MR when the full test suite runs.