Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create SimpleFile only when writing the content #19493

Merged
merged 4 commits into from
Feb 28, 2020
Merged

Conversation

icewind1991
Copy link
Member

instead of first creating an empty file and then writing the content.

This solves the overhead of creating an empty file with the common pattern:

$file = $simpleFilder->newFile('foo.txt');
$file->putContent('bar.txt');

roughly halving the number of storage and database operations that need to be done when creating a SimpleFile.

This is not automatically done with File because that has a more complex api which I'm more hesitant to touch.
Instead the Folder::newFile api has been extended to accept the content for the new file.

In my local testing, the overhead of first creating an empty file took about 20% of the time for preview generation

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Feb 16, 2020
@icewind1991 icewind1991 added this to the Nextcloud 19 milestone Feb 16, 2020
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 👍 Just a small comment, but nothing necessary.

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lib/private/Files/SimpleFS/NewSimpleFile.php Outdated Show resolved Hide resolved
lib/private/Files/Node/Folder.php Show resolved Hide resolved
@icewind1991
Copy link
Member Author

all done

@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 25, 2020
icewind1991 and others added 4 commits February 28, 2020 12:55
test behavior, not implementation

Signed-off-by: Robin Appelman <[email protected]>
instead of first creating an empty file and then writing the content.

This solves the overhead of creating an empty file with the common pattern:

```php
$file = $simpleFilder->newFile('foo.txt');
$file->putContent('bar.txt');
```

roughly halving the number of storage and database operations that need to be done when creating a `SimpleFile`.

This is not automatically done with `File` because that has a more complex api which I'm more hesitant to touch.
Instead the `Folder::newFile` api has been extended to accept the content for the new file.

In my local testing, the overhead of first creating an empty file took about 20% of the time for preview generation

Signed-off-by: Robin Appelman <[email protected]>
Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer
Copy link
Member

rullzer commented Feb 28, 2020

rebased and bumped autoloader

@rullzer rullzer merged commit de34786 into master Feb 28, 2020
@rullzer rullzer deleted the simplefile-new-lazy branch February 28, 2020 15:07
@nickvergessen
Copy link
Member

Regressions:

  • PhotoCache in DAV always checks if there is a valid photo
  • Generated avatars are not updated when the user display name is set/changed

nickvergessen added a commit that referenced this pull request Mar 24, 2020
Since #19493 newFile() by default does not write the file anymore.
Setting an empty string as content will make it write the file immediate,
without having to call putContent() afterwards.

Signed-off-by: Joas Schilling <[email protected]>
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants