Use multimap with Persistent module handle to avoid IdentityHash collision #1466
Conversation
| std::string referrer_filename = d->module_filename_map_[ref_id]; | ||
| auto range = d->module_info_map_.equal_range(ref_id); | ||
| std::string referrer_filename = ""; | ||
| for (auto it = range.first; it != range.second; ++it) { |
There was a problem hiding this comment.
If I understand this correctly, this loop should almost always end after the first iteration?
There was a problem hiding this comment.
Most of the times if there is no hash collision.
| std::map<int, std::string> module_filename_map_; | ||
| // identity hash -> filename, module (avoid hash collision) | ||
| std::multimap<int, std::pair<std::string, v8::Persistent<v8::Module>>> | ||
| module_info_map_; | ||
| // filename -> Module | ||
| std::map<std::string, v8::Persistent<v8::Module>> module_map_; |
There was a problem hiding this comment.
I don't like that there's two Persistent<Module> references now. I don't understand why module_filename_map_ was changed to include the Persistent<Module>...
There was a problem hiding this comment.
It is used to validate that the filename is indeed the one from the module: there might be multiple entries with the same hash but from different modules, and keeping the reference allows comparing if the identity matches (since we also have a reference to referrer when this map is queried)
There was a problem hiding this comment.
You haven't actually experience a hash collision yet? I would wait until that happens to introduce extra complexity. It seems likely that we will never experience it and due to the
CHECK_EQ(0, module_filename_map_.count(id));
We'll see a crash if it happens, so it will be fairly obvious and easy to fix.
There was a problem hiding this comment.
Hmm... personally I prefer preventive approaches, but yeah it is also an option to postpone until it really happens...
(Or at least we could leave a comment here about possible ways to address this issue if it ever happens...)
There was a problem hiding this comment.
Just had a discussion with TimothyGu (former Node TSC) about this, and he said that there were actually occasions when they observed IdentityHash collision, and it is never designed to be unique. He also suggested the Node way of keeping such persistent handles.
There was a problem hiding this comment.
Ok. Let's do it. I have a few minor issues but otherwise looks good.
Cool glad to hear |
| auto range = d->module_info_map_.equal_range(ref_id); | ||
| std::string referrer_filename = ""; | ||
| for (auto it = range.first; it != range.second; ++it) { | ||
| if (it->second.second == referrer) { |
There was a problem hiding this comment.
Here the == is comparing v8 persistent objects? Please add a comment to that effect.
There was a problem hiding this comment.
It is overloaded to compare the value inside the handle. (similar to Local ==, which is well documented. Their implementation is very similar)
https://denolib.github.io/v8-docs/include_2v8_8h_source.html#l00487
I'll add a comment on this.
Avoids possible identity hash collision by making the map a multimap and keep a persistent handle for the module to compare.
This construct is also seen in Node.
(Tried running this on top of the ESM PR, and seems fine with module resolutions)
(The O(1000) comments seems valid (tested myself), but considering the hash is an int, the collision chance should still be quite high...)