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

MM-7633: Optimize memory utilization during file uploads #9835

Merged
merged 14 commits into from Dec 13, 2018

Conversation

Projects
None yet
10 participants
@levb
Copy link
Contributor

levb commented Nov 16, 2018

Summary

Refactored the file upload code to reduce redundant buffering and stream
directly to the file store. Added tests.

Benchmark results:

levs-mbp:mattermost-server levb$  go test -v -run nothing -bench Upload -benchmem ./app
...
BenchmarkUploadFile/random-5Mb-gif-raw-ish_DoUploadFile-4         	      10	 122598031 ns/op	21211370 B/op	    1008 allocs/op
BenchmarkUploadFile/random-5Mb-gif-raw_UploadFileTask-4           	     100	  20211926 ns/op	 5678750 B/op	     126 allocs/op
BenchmarkUploadFile/random-5Mb-gif-UploadFiles-4                  	       2	1037051184 ns/op	81806360 B/op	 3705013 allocs/op
BenchmarkUploadFile/random-5Mb-gif-UploadFileTask-4               	       2	 933644431 ns/op	67015868 B/op	 3704410 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-raw-ish_DoUploadFile-4         	     100	  13110509 ns/op	 6032614 B/op	    8052 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-raw_UploadFileTask-4           	     100	  10729867 ns/op	 1738303 B/op	     125 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-UploadFiles-4                  	       2	 925274912 ns/op	70326352 B/op	 3718856 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-UploadFileTask-4               	       2	 995033336 ns/op	58113796 B/op	 3710943 allocs/op
BenchmarkUploadFile/zero-10Mb-raw-ish_DoUploadFile-4              	      30	  50777211 ns/op	54791929 B/op	    2714 allocs/op
BenchmarkUploadFile/zero-10Mb-raw_UploadFileTask-4                	      50	  36387339 ns/op	10503920 B/op	     126 allocs/op
BenchmarkUploadFile/zero-10Mb-UploadFiles-4                       	      30	  48657678 ns/op	54791948 B/op	    2719 allocs/op
BenchmarkUploadFile/zero-10Mb-UploadFileTask-4                    	      50	  37506467 ns/op	31492060 B/op	     131 allocs/op
...

Ticket

https://mattermost.atlassian.net/browse/MM-7633 #7801

Checklist

  • Added or updated unit tests (required for all new features)
  • Added API documentation (required for all new APIs)
  • All new/modified APIs include changes to the drivers
    N/A???
  • Includes text changes and localization file (.../i18n/en.json) updates

Details

Overview of changes:

  • api4
    • Replaced uploadFile handler with uploadFileStream that reduces
      unnecessary buffering.
    • Added/refactored tests for the new API.
    • Refactored apitestlib/Check...Status functions.
  • app
    • Added App.UploadFileTask, a more efficient refactor of UploadFile.
    • Consistently set FileInfo.HasPreviewImage
    • Added benchmarks for the new and prior implementations
    • Replaced passing around *image.Image with image.Image in the
      existing code.
  • model
    • Added a more capable client4.UploadFiles API to match the new server
      API’s capabilities.
  • I18n
    • Replaced api.file.upload_file.bad_parse.app_error with a more generic
      api.file.upload_file.read_request.app_error
  • plugin
    • Removed type plugin.multiPluginHookRunnerFunc in favor of using
      func(hooks Hooks) bool explicitly, to help with testing
  • tests
    • Added test files for testing images

Still remaining, but can be separate PRs - please let me know the preferred
course of action
- Investigate JS client API - how does it do multipart?
- Performance loss from old code on (small) image processing?
- Deprecate the old functions, change other API implementations to use
UploadFileTask

Definitely separate future PRs - should I file tickets foe these?
- Only invoke t.readAll() if there are indeed applicable plugins to run
- Find a way to leverage goexif buffer rather than re-reading

Suggested long-term improvements - should I file separate tickets for these?

  • Actually allow uploading of large (GB-sized) files. This may require a
    change in how the file is passed to plugins.
  • (Many) api4 tests should probably be subtests and share a server setup -
    will be much faster
  • Performance improvements in image processing (goexif/thumbnail/preview)
    (maybe use https://mattermost.atlassian.net/browse/MM-10188 for this)

Questions:

  1. I am commiting MBs of test images, are there better alternatives? I can
    probably create much less dense images that would take up considerably less
    space, even at pretty large sizes
  2. I18n: Do I need to do anything special for the string change? Or just wait
    until it gets picked up and translated/updated?
  3. The image dimensions are flipped in resulting FileInfo to match the actual
    orientation. Is this by design? Should add a test for it, perhaps?
  4. What to do in the case of partial success? S3 but not DB, some files but not
    others? For now, just doing what the old code did, I think.
  5. Make maxUploadDrainBytes configurable? Also, should this be the systemic
    behavior of all APIs with non-empty body? Otherwise dropped altogether?
    Check all other ioutil.ReadAll() from sockets. Find a way to set a total
    byte limit on request Body?
@mattermod

This comment has been minimized.

Copy link

mattermod commented Nov 16, 2018

Thanks @levb for the pull request!

Per the CONTRIBUTING.md file displayed when you created this pull request, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?

This is a standard procedure for many open source projects. Your form should be processed within 24 hours and reviewers for your pull request will be able to proceed.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

@levb levb force-pushed the levb:MM-7633 branch 4 times, most recently from b7f65f3 to 193f76b Nov 16, 2018

@jwilander jwilander requested review from lieut-data and crspeller Nov 16, 2018

@levb levb force-pushed the levb:MM-7633 branch 2 times, most recently from 3f1c84f to c58ca76 Nov 20, 2018

@crspeller
Copy link
Member

crspeller left a comment

Overall this is a great step forward for optimizing file uploads. It adds quite a bit of complexity, but I think it's handed decently.

Your questions:

  1. Reduced file sizes would be nice. I don't think it's a strict requirement.
  2. No as long as you update en.json the translation system will take care of the rest.
  3. I think that is intentional @hmhealey would be able to confirm.
  4. As long as an error is reported to the user, the system should be pretty resilient about having bad data in the DB or on S3 so it's not a problem.
  5. I think investigating that possibility would be a good follow up ticket.

A follow up ticket for the webapp and RN app to make sure they are using the best method would be great. We should also think about removing the fallback code if we move to APIv5 as it does add a bit of complexity.
A follow up ticket to investigate improvements to the plugin API for file uploads would also be good.

Show resolved Hide resolved api4/file.go Outdated
Show resolved Hide resolved app/file.go Outdated
@hmhealey

This comment has been minimized.

Copy link
Member

hmhealey commented Nov 21, 2018

Do you mean that the image dimensions match the original orientation and not the dimensions of the image after we've made it upright? Like if you have an image with a width of 200 and a height of 300 that is rotated by 90 degress, it still saves width = 200 and height = 300 even though the file on disk has those dimensions flipped? If that's the case, I think that would be a bug

@levb

This comment has been minimized.

Copy link
Contributor Author

levb commented Nov 21, 2018

@hmhealey It is my understanding that we upload the original image "as is", we do not flip it since it already contains the necessary metadata, and all user agents respect it. However, if the image was flipped, we seem to be recording its dimensions in FileInfo to match the actual display.

image size orientation FileInfo size
uploaded image WxH 5 (90º?) n/a
stored image WxH 5 (90º?) HxW
thumb calculated, from HxW 1 (normal) n/a
preview calculated, from HxW 1 (normal) n/a

I figured that perhaps we were using the FileInfo dimensions for sizing UI elements before the image is downloaded, so they had to be "real"? If you can point me at where/how they are used, maybe we can add a test from that?

@hmhealey

This comment has been minimized.

Copy link
Member

hmhealey commented Nov 23, 2018

Oh, I understand now. I forgot we preserve the original file while just rotating the thumbnail/preview to be upright, and we use those dimensions in the web app to lay out the post in places like this one

@hmhealey

This comment has been minimized.

Copy link
Member

hmhealey commented Nov 23, 2018

Maybe file a ticket to look into correcting that so we can spend some more time thinking about it? Perhaps we could add an orientation field to the FileInfo and make it so that the width/height always match the native values from the image, although that would have to be done with enough time to ensure that the mobile app is backwards compatible with those changes (mostly for the case of an old app with a new server)

@lieut-data
Copy link
Member

lieut-data left a comment

@levb, sorry for the delay in providing feedback on this one. I've thrown up some preliminary comments (mostly nits at this point), but I'm going to some more time to ingest this properly to complete the review.

Show resolved Hide resolved api4/apitestlib.go Outdated
Show resolved Hide resolved api4/apitestlib.go
Show resolved Hide resolved api4/file.go Outdated
Show resolved Hide resolved app/file.go Outdated

@lieut-data lieut-data self-requested a review Nov 26, 2018

@levb

This comment has been minimized.

Copy link
Contributor Author

levb commented Nov 26, 2018

@hmhealey I think we should leave it as is for now, and I'll add a test that documents/verifies the behavior. When we look into cleaning up image processing performance issues, we can look into maybe adding the thumbnail/preview sizes to FileInfo explicitly, that way the client doesn't need to guess-calculate them.

@levb levb force-pushed the levb:MM-7633 branch 3 times, most recently from 90c95c1 to da56977 Nov 26, 2018

@lieut-data
Copy link
Member

lieut-data left a comment

I haven't dug into UploadFileTask in depth yet (since the first review), but wanted to submit some comments for initial feedback :)

Show resolved Hide resolved api4/apitestlib.go
Show resolved Hide resolved api4/file.go Outdated
Show resolved Hide resolved api4/file.go Outdated
Show resolved Hide resolved api4/file.go Outdated
Show resolved Hide resolved api4/file.go Outdated
Show resolved Hide resolved api4/file.go
Show resolved Hide resolved api4/file.go Outdated
Show resolved Hide resolved api4/file.go Outdated
Show resolved Hide resolved app/file.go Outdated
Show resolved Hide resolved app/file.go Outdated
@lieut-data

This comment has been minimized.

Copy link
Member

lieut-data commented Dec 2, 2018

The other high level comment I wanted to add is that: is there any way to simplify the assumptions? There's a good deal of complexity here to preserve 100% compatibility with the existing API, but what if we exposed a new API with tighter semantics: no application/x-www-form-urlencoded uploads, all channel_id parameters before file parameters (or just in the query string?) -- basically anything we wanted so that the resulting implementation was simpler, perhaps even to the point of dropping the buffered code path in the new API.

This would mean maintaining the old API until the next major version upgrade (and coordinating with the mobile app around this), but maybe it would be worth it?

@levb levb force-pushed the levb:MM-7633 branch from da56977 to bbc2b1b Dec 3, 2018

@levb

This comment has been minimized.

Copy link
Contributor Author

levb commented Dec 3, 2018

@lieut-data I appreciate the advantages of simplifying the new API by keeping the backwards compatibility out of it. The main reason I decided to include the "legacy" path in the new implementation so that we can cleanly switch the various file upload use-cases to the new API and replace all of the old code. It was also nice for testing not to have 2 separate end-points to test.

With the changes you hinted at, I replaced the obscure request hacking with a couple small functions to make our own MultipartReader and I think it should be easier to read now. Let me know if you still feel reserved about the backwards compatibility issue.

@lieut-data

This comment has been minimized.

Copy link
Member

lieut-data commented Dec 4, 2018

Sounds great, @levb! Will take another pass :)

@levb levb force-pushed the levb:MM-7633 branch 2 times, most recently from b48ce87 to 230859b Dec 7, 2018

@levb levb requested review from lieut-data and crspeller Dec 12, 2018

@lieut-data
Copy link
Member

lieut-data left a comment

Terrific simplifications, @levb! I've added a few more comments to the code, but my only outstanding concern is the exported UploadFiles interface in Client4. Details below.

Show resolved Hide resolved app/file_test.go Outdated
Show resolved Hide resolved api4/file.go Outdated
Show resolved Hide resolved api4/file.go Outdated
Show resolved Hide resolved api4/file.go
Show resolved Hide resolved api4/file.go
app/file.go Outdated
ImageThumbnailRatio = float64(ImageThumbnailHeight) / float64(ImageThumbnailWidth)
ImagePreviewWidth = 1920

MinUploadFileBufferSize = 2 * 1024 * 1024

This comment has been minimized.

@lieut-data

lieut-data Dec 12, 2018

Member
Suggested change
MinUploadFileBufferSize = 2 * 1024 * 1024
MinUploadFileBufferSize = 2 * 1024 * 1024 // 2Mb
@@ -421,6 +431,407 @@ func (a *App) DoUploadFile(now time.Time, rawTeamId string, rawChannelId string,
return info, err
}

func UploadFileSetTeamId(teamId string) func(t *uploadFileTask) {

This comment has been minimized.

@lieut-data

lieut-data Dec 12, 2018

Member

Interesting pattern. 0/5, but I wonder if UploadFileX should just accept UploadFileTask directly, and these can be methods on the struct as such.

This comment has been minimized.

@levb

levb Dec 13, 2018

Author Contributor

Once the functional options were exposed, I did not see the need to expose the struct, especially not its fields creating 2 different usage patterns. As to exposing functions returning non-public types - I was surprised myself that Go let me get away with it, but it did. UploadFileSetXXX(teamId string) func(t *uploadFileTask) - used in api4/file.go. I would prefer that we leave it as is for now, but would be more than happy to (a) engage in a discussion on the preferred method, and if we find it useful spearheading the effort to converge on it, as practical, no excesses.

(also, the name UploadFileX is a placeholder, (2/5?) I think we should soon deprecate the old functions and eventually replace everything with a single App.UploadFile() fit for most if not all upload use cases)

Show resolved Hide resolved app/file.go
Show resolved Hide resolved app/file.go Outdated
}
}

func (c *Client4) UploadFiles(

This comment has been minimized.

@lieut-data

lieut-data Dec 12, 2018

Member

UploadFiles will now be an official part of the Client4 API. Is this the signature we want to support going forward until the next break as such? Thoughts:

  • Maybe force useMultipart?
  • Maybe just accept io.Reader (do we need to be able to close it?), and let the end user handle the complexity of passing a file handler? It seems like we support streaming the file without knowing the size anyway, so maybe we drop contentLengths too?
  • Would it make sense to generate our own client ids and return them to the client instead? Or do we need the at all? I'm actually not sure what the clientIds are used for anyway, other than the implementation specifics of how the webapp happens to upload files.

This comment has been minimized.

@levb

levb Dec 13, 2018

Author Contributor

@lieut-data I submitted fixes for your other comments. I agree with your reservation about exposing client4.UploadFiles as is. I designed it to be a "swiss army knife" function, matching the server API so that it can be useful for comprehensive API testing. We could just this code in the api4/..._test.go for now, until we work out the API quirks.

Specific suggestions:

  1. +1 to exclusively using mime/multipart for all file upload use cases (including special images, plugins, etc) - that's the browser default anyway. Would be a nice simplification. However it is IMO important that the client/server implementations match so we should then remove simple POST support throughout. Not sure how we need to/can ensure backwards compatibility.

  2. If we switch to multipart exclusively, content-length on individual parts is unavailable and makes little sense to support in the API. If we continue supporting simple POST, I'd argue we should keep content-length, seems like the best practice.

  3. My understanding of the purpose of client_id was different. I thought this was a 3rd party key that we'd save with the file so it can be retrieved again from that 3rd party app/usecase. Fopr this, it must be provided by the customer/3rd party app. Why would our own client need a yet another Id? I would +1 the effort to deprecate the concept until we have a use case for it.

  4. Opening and closing files just-in-time was just me being perfectionist, seemed wrong to hold on to all the file descriptors while they are uploaded one at a time. Like, if we wanted to run concurrent benchmarks, etc.

If you agree to move it to _test for now, lmk. If you think we should keep this as a public API, let's maybe have a quick chat to sync up on how to best do it?

This comment has been minimized.

@lieut-data

lieut-data Dec 13, 2018

Member
  1. I agree that we need to maintain backwards compatibility for the HTTP REST API, but this is just an abstraction on top of that to aid integrators. I don't think there's any expectation that the abstraction be as flexible as the underlying functionality -- and developers can always use the HTTP REST API directly if needed, which I agree we shouldn't break. 4/5.
  2. I think the arguments in 1. apply here, in that I don't think we need to support simple POST for this particular abstraction layer, and thus by extension don't need content-length as a parameter.
  3. 100% agree, at least until there's a use case for exposing this again. (Still supported by the underlying HTTP REST API of course.)
  4. 0/5 on this at the moment :)

I really like your suggestion to move this into _test for now and remove it from the public API, allowing us to iterate further. 👍

@levb levb requested a review from lieut-data Dec 13, 2018

@lieut-data
Copy link
Member

lieut-data left a comment

I'm good with merging these changes to master, but suggest we iterate on the Client4 API surface to make sure this is what we want to support for non-plugin developers, etc.

@lieut-data lieut-data added this to the v5.8.0 milestone Dec 13, 2018

Lev Brouk and others added some commits Oct 7, 2018

MM-7633: Optimize memory utilization during file uploads
Refactored the file upload code to reduce redundant buffering and stream
directly to the file store. Added tests.

Benchmark results:
```
levs-mbp:mattermost-server levb$  go test -v -run nothing -bench Upload -benchmem ./app
...
BenchmarkUploadFile/random-5Mb-gif-raw-ish_DoUploadFile-4         	      10	 122598031 ns/op	21211370 B/op	    1008 allocs/op
BenchmarkUploadFile/random-5Mb-gif-raw_UploadFileTask-4           	     100	  20211926 ns/op	 5678750 B/op	     126 allocs/op
BenchmarkUploadFile/random-5Mb-gif-UploadFiles-4                  	       2	1037051184 ns/op	81806360 B/op	 3705013 allocs/op
BenchmarkUploadFile/random-5Mb-gif-UploadFileTask-4               	       2	 933644431 ns/op	67015868 B/op	 3704410 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-raw-ish_DoUploadFile-4         	     100	  13110509 ns/op	 6032614 B/op	    8052 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-raw_UploadFileTask-4           	     100	  10729867 ns/op	 1738303 B/op	     125 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-UploadFiles-4                  	       2	 925274912 ns/op	70326352 B/op	 3718856 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-UploadFileTask-4               	       2	 995033336 ns/op	58113796 B/op	 3710943 allocs/op
BenchmarkUploadFile/zero-10Mb-raw-ish_DoUploadFile-4              	      30	  50777211 ns/op	54791929 B/op	    2714 allocs/op
BenchmarkUploadFile/zero-10Mb-raw_UploadFileTask-4                	      50	  36387339 ns/op	10503920 B/op	     126 allocs/op
BenchmarkUploadFile/zero-10Mb-UploadFiles-4                       	      30	  48657678 ns/op	54791948 B/op	    2719 allocs/op
BenchmarkUploadFile/zero-10Mb-UploadFileTask-4                    	      50	  37506467 ns/op	31492060 B/op	     131 allocs/op
...
```

https://mattermost.atlassian.net/browse/MM-7633 #7801

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]
- [x] Added or updated unit tests (required for all new features)
- [ ] Added API documentation (required for all new APIs)
- [ ] All new/modified APIs include changes to the drivers
  *N/A*???
- [x] Includes text changes and localization file ([.../i18n/en.json](https://github.com/mattermost/mattermost-server/blob/master/i18n/en.json)) updates

Overview of changes:
- api4
    - Replaced `uploadFile` handler with `uploadFileStream` that reduces
      unnecessary buffering.
    - Added/refactored tests for the new API.
    - Refactored apitestlib/Check...Status functions.
- app
    - Added App.UploadFileTask, a more efficient refactor of UploadFile.
    - Consistently set `FileInfo.HasPreviewImage`
    - Added benchmarks for the new and prior implementations
    - Replaced passing around `*image.Image` with `image.Image` in the
      existing code.
- model
    - Added a more capable `client4.UploadFiles` API to match the new server
      API’s capabilities.
- I18n
    - Replaced `api.file.upload_file.bad_parse.app_error` with a more generic
      `api.file.upload_file.read_request.app_error`
- plugin
    - Removed type `plugin.multiPluginHookRunnerFunc` in favor of using
      `func(hooks Hooks) bool` explicitly, to help with testing
- tests
    - Added test files for testing images

Still remaining, but can be separate PRs - please let me know the preferred
course of action
    - Investigate JS client API - how does it do multipart?
    - Performance loss from old code on (small) image processing?
    - Deprecate the old functions, change other API implementations to use
      UploadFileTask

Definitely separate future PRs - should I file tickets foe these?
    - Only invoke t.readAll() if there are indeed applicable plugins to run
    - Find a way to leverage goexif buffer rather than re-reading

Suggested long-term improvements - should I file separate tickets for these?
- Actually allow uploading of large (GB-sized) files. This may require a
  change in how the file is passed to plugins.
- (Many) api4 tests should probably be subtests and share a server setup -
  will be much faster
- Performance improvements in image processing (goexif/thumbnail/preview)
  (maybe use https://mattermost.atlassian.net/browse/MM-10188 for this)

Questions:
1. I am commiting MBs of test images, are there better alternatives? I can
   probably create much less dense images that would take up considerably less
   space, even at pretty large sizes
2. I18n: Do I need to do anything special for the string change? Or just wait
   until it gets picked up and translated/updated?
3. The image dimensions are flipped in resulting FileInfo to match the actual
   orientation. Is this by design? Should add a test for it, perhaps?
4. What to do in the case of partial success? S3 but not DB, some files but not
   others? For now, just doing what the old code did, I think.
5. Make maxUploadDrainBytes configurable? Also, should this be the systemic
   behavior of all APIs with non-empty body? Otherwise dropped altogether?
   Check all other ioutil.ReadAll() from sockets. Find a way to set a total
   byte limit on request Body?
WIP - PR feedback
1. Refactored checkHTTPStatus to improve failure messages
2. Use `var name []type` rather than `name := ([]type)(nil)`
3. Replaced single-letter `p` with a more intention-revealing `part`
4. Added tests for full image size, `HasPreviewImage`
WIP - PR feedback: eliminated use of Request.MultipartReader
Instead of hacking the request object to use r.MultipartReader now have own
functions `parseMultipartRequestHeader` and `multipartReader` eliminating the
need to hack the request object to use Request.MultipartReader limitations.

@levb levb force-pushed the levb:MM-7633 branch from 24bbb63 to df7f1ce Dec 13, 2018

@cpanato cpanato merged commit fae4f60 into mattermost:master Dec 13, 2018

2 of 3 checks passed

build.mattermost.com Failure in Build
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
mattermost/serverless-ops/github PR is up-to-date.

@levb levb deleted the levb:MM-7633 branch Dec 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.