perf(h5p-server): reduce memory and IO when uploading package #2100
Conversation
Reference a single file for identical filepaths rather than one file for each.
|
Just adding: BookMaker is a non-public content type that's re-purposing CoursePresentation: Removing plenty of things, adding some new, but basically CoursePresentation. Might help for reproducing the initial issue. |
|
Thanks, Oliver. Can't keep track of them all 😅 |
|
@cabauman Thank you for the PR! It looks like a very sensible addition and I'd be happy to merge it. If you could add a test that covers the new lines, it is ready to be merged. |
| @@ -236,12 +236,18 @@ export default class ContentStorer { | |||
| ) | |||
| ); | |||
|
|
|||
| const filepathTonewFilenameMap = new Map<string, string>(); | |||
There was a problem hiding this comment.
should probably be filepathToNewFilenameMap
There was a problem hiding this comment.
Good catch. I'll also go ahead and change the filepath part to filePath for consistency with the rest of the code base.
|
Great. Let me know if there's anything you'd like me to tweak in the tests. |
|
@cabauman Thanks that you are donating your time! |
|
@otacke It's a pleasure. I love the open source community. |
|
Thank you for the contribution! |
Reference a single file for identical file paths rather than one file for each. This improvement is limited to content types with copy/paste functionality.
So this came about when I was interacting with a Book Maker activity that has numerous images with 20 copies each (along with audio files for picked up and dropped). When I exported from WordPress and uploaded to Lumi, I noticed a given image/audio count jumps from one to the number of copies. On the other hand, creating a new activity in Lumi with copy/pasted images references a single file. So it's only the upload feature that does this. Next, I tried the reverse to see how WordPress handles uploaded packages, and discovered that one image file remained as one file. So I figured it would be nice to bring that optimization here too.
I tested my changes manually, and it seems to work well. The following screenshot is the result of uploading a package with 8 copy/pasted images before and after the change:
If you guys welcome this change, I'll look into adding tests to go along with this PR. Off to bed for now!
Thanks for all the hard work on this project.