-
Notifications
You must be signed in to change notification settings - Fork 24
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
Speed up JSON mappings #7706
Speed up JSON mappings #7706
Conversation
… extra mappingKeys array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome speed up 🕺 I left a couple of thoughts, but probably the code can stay as is. let me know if this is the case, then I'll test.
@@ -555,7 +555,7 @@ class DataApi { | |||
* | |||
* api.setMapping("segmentation", mapping); | |||
*/ | |||
setMapping(layerName: string, mapping: Mapping) { | |||
setMapping(layerName: string, mapping: Mapping | Record<number, number>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we call this code internally, right? (e.g., in merger mode) then, it might make sense to avoid the copying in lines 567f somehow 🤔 what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need to do the copying for our internal usages, because the merger mode will continue to modify the map after it was added to the store which we'd like to prevent 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also see think that it would be way cleaner to avoid mutating the mapping that is already active in the store. At the same time, I'm wondering whether there would be a visible bug, because the mutated mapping is set to the store afterwards, anyway. But obviously we should only do such things, when the performance win is interesting to us. For the merger mode, this is not the case, I think. Not sure whether similar thoughts can be had for your front-end-mapping PR.. However, if you feel like performance is good enough now, let's stick to that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the frontend mapping PR, the api functionality is not used. Instead the setMappingAction is dispatched directly. I guess one could do the same in the merger mode too, but as you said, it doesn't seem performance critical for typical merger mode scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works well 💯 also, great clean up by dropping the mapping keys 👍
Also, no longer maintain the extra mappingKeys array which was only needed, because Object.keys was very slow for large objects. I benchmarked the old code, the new code, and the new code maintaining the extra mappingKeys array (all with deepFreeze disabled!) using a large JSON mapping with 1.5M entries:
Object mapping with extra keys array (old):
Mapping creation: ~4600 ms
Mapping texture creation: ~60 ms
Map mapping with extra keys array (intermediate):
Mapping creation: ~1160 ms
Mapping texture creation: ~460 ms
Map mapping (new):
Mapping creation: ~590 ms
Mapping texture creation: ~1070 ms
Overall a nice 3x speedup for larger JSON mappings. However, I mainly did this in preparation for #7654 where this frontend mapping functionality will be used extensively.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)