Replace map with unordered map

[markccchiang]

Description

• What is implemented or fixed? Mention the linked issue(s), if any.
  Closes Replace map with unordered_map #1295. Being a pure lookup table, unordered_map is supposed to perform better than map. According to the discussions from the stake overflow. However, using unordered_map would need more memory space than that for map. The new tester TestMapPerf simply tests the performances of map and unordered_map. unordered_map is a little bit faster than map when inserting a new data element or finding & copying a specific data element. But it is significantly slower than map when erasing map elements or manipulating the map structure. Ref: map vs. unordered_map in C++.

• How does this PR solve the issue? Give a brief summary.
  Replace all map with unordered_map, except for the ordered array from HttpServer.

• Are there any companion PRs (frontend, protobuf)?
  No.

• Is there anything else that testers should know (e.g. exactly how to reproduce the issue)?
  It will not affect the original tests. @acdo2002 could you please check if there are any performance differences compared to the dev branch?

Checklist

• changelog updated / no changelog update needed
• e2e test passing / corresponding fix added / new e2e test created
• protobuf updated to the latest dev commit / no protobuf update needed
• protobuf version bumped / protobuf version not bumped
• added reviewers and assignee
• added ZenHub estimate, milestone, and release

[github-actions]

Package	Line Rate	Health
src.Cache	66%	➖
src.DataStream	52%	➖
src.FileList	67%	➖
src.Frame	50%	➖
src.HttpServer	42%	➖
src.ImageData	28%	❌
src.ImageFitter	83%	✔
src.ImageGenerators	43%	➖
src.ImageStats	76%	✔
src.Logger	38%	➖
src.Main	52%	➖
src.Region	18%	❌
src.Session	30%	❌
src.Table	52%	➖
src.ThreadingManager	87%	✔
src.Timer	87%	✔
src.Util	48%	➖
Summary	38% (7094 / 18743)	❌

[kswang1029]

I simply run e2e tests and no regression is spotted. If there are specific parts I need to pay attend to, please let me know. Thanks.

[kswang1029]

@kswang1029 to do performance test via e2e tests. check test elapsed time!

[kswang1029]

@acdo2002 please execute ICD test manually in a sequence (not in parallel) and compare elapsed time of each test to see if there is significant performance boost or degradation. I will do a similar one but from e2e perspective. @confluence these test results would provide a hint about performance impact hopefully.

[kswang1029]

@markccchiang @confluence Not sure if I am seeing a thing here. It appears to me that dev (with map) performs better than unordered_map branch. Basically all tests with unordered_map branch require more time to execute.

[confluence]

Hmm. In that case, I suggest that we move this to draft until I can dig further into the code and look at how we're using the maps in different components. It's possible that only some of the maps should be changed, none of them should be changed, or that some of our unordered maps should be ordered maps! Clearly it's not as simple as changing them across the board.

[kswang1029]

Hmm. In that case, I suggest that we move this to draft until I can dig further into the code and look at how we're using the maps in different components. It's possible that only some of the maps should be changed, none of them should be changed, or that some of our unordered maps should be ordered maps! Clearly it's not as simple as changing them across the board.

Any naive explanations why?

[confluence]

Any naive explanations why?

My short answer is: the way we're accessing the maps is (on average!) triggering cases where map is better more than the cases where map is worse. Some guesses:

• unexpected hash collisions (could be a bug in a hash function we've defined -- that's happened before!)
• something else to do with key hashing (e.g. we have a complicated key type and the hashing and/or equality comparison is expensive)
• we're performing one of the slow operations more often than I thought

But these are just guesses, and this requires further investigation. It's possible that only one of these maps is the source of the slowdown, and all the other maps do in fact benefit from being converted (but the one slow map is disproportionately affecting the outcome because it's accessed a lot more frequently).

(Edit: my best guess is that using the StatsRegionId struct as a key, with custom hashing and comparison functions, is expensive. I have an idea for testing this theory, but I need to think about how to implement it correctly.) This is only used in a very limited way, so it's probably not that. String keys may be expensive to hash, though, and we use those in a few places in the metadata code.

[confluence]

I've also noticed that some of the region code repeatedly creates and destroys local variables which are small maps, and this might be more expensive for unordered maps. But I also don't know if this is happening frequently enough to be an issue. Rather than continuing to guess, I will try to use a profiler. :)

[kswang1029]

I've also noticed that some of the region code repeatedly creates and destroys local variables which are small maps, and this might be more expensive for unordered maps. But I also don't know if this is happening frequently enough to be an issue. Rather than continuing to guess, I will try to use a profiler. :)

indeed, region e2e tests do have the performance degradation problem more significantly. All the e2e tests have a set of common step: load file list, load file info, and then load image or images.

[markccchiang]

Hmm. In that case, I suggest that we move this to draft until I can dig further into the code and look at how we're using the maps in different components. It's possible that only some of the maps should be changed, none of them should be changed, or that some of our unordered maps should be ordered maps! Clearly it's not as simple as changing them across the board.

Yes. Considering the current performance test results, we may switch between map and unordered_map one by one, and see which is better. This is trivial but time-consuming work -- so far there is no clear guide on which kind of map is better for us to use.

[acdo2002]

I did a quick test, here is the result.
the current dev
current performance regression tests (not much region related, big 35 GB fits, casa and 70GB hdf5):102.882 s, 100.716 s, 100.641 s, 100.945 s, 100.977 s
ICD-RxJS (small image size) region related tests:3.341 s, 3.442 s, 3.49 s, 3.465 s, 3.473 s

the unordered map branch:
current performance regression tests (not much region related, big 35 GB fits, casa and 70GB hdf5):101.107 s, 101.149 s
101.007 s, 100.931 s, 100.984 s => NO big difference
ICD-RxJS (small image size) region related tests:3.535 s, 3.48 s, 3.467 s, 3.498 s, 3.527 s => little bit slower, perhaps need the specific large region sizes test rather than bigger image size tests

NOTE: there is a regression failure happened on SET_REGION_ANNOTATION_EXPORT_IMPORT_CASA_PIXEL.test.ts
The exported region does not match to the expect
Expected: 163
Received: 157.71336364746094