Asking for memory report can cause a virtual iloop comparing strings in JS CollectReports()
Categories
(Core :: JavaScript Engine, defect, P3)
Tracking
()
People
(Reporter: jesup, Unassigned, NeedInfo)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Reporter | ||
Comment 2•7 years ago
|
||
Comment 3•7 years ago
|
||
![]() |
||
Comment 4•7 years ago
|
||
Comment 5•7 years ago
|
||
Reporter | ||
Comment 6•7 years ago
|
||
Reporter | ||
Comment 7•6 years ago
|
||
This profile https://perfht.ml/2SswAv8 shows we can still spend crazy amounts of time collecting reports; we need to have some way to limit the time spent de-dupping strings for memory reports. (I've measured it taking 5-30+ minutes doing this.... effectively the process is locked)
Comment 8•6 years ago
|
||
Jan, do you have any thoughts on what the next steps with this bug are? Is this a GC related bug?
Comment 9•6 years ago
|
||
Comment 10•6 years ago
|
||
Depends on D38894
Comment 11•6 years ago
|
||
Hmm, I'm wondering if doing only partial hashes instead of hashing the complete string is possible here. In addition to that, it should be possible to optimise the string comparison for certain cases, like for example when comparing a JSRope
against a JSLinearString
. See the attached WIP patches for both ideas.
Do we have any reproducible test cases where collecting the memory report takes too long?
![]() |
||
Comment 12•6 years ago
|
||
It's unrelated to GC. Memory reporting looks for duplicated strings so it can mention them, because that can be indicative of interesting problems, but I don't know how useful it is in practice. A reasonable option would be to remove it entirely, which would solve the performance problem completely.
Reporter | ||
Comment 13•6 years ago
|
||
Well, I have used this feature to identify a number of site-specific memory leaks/problems - adsafeprotected generating 100's of K of copies of 100-200-char strings; Facebook generating 500K copies of "<DIV", etc. So I'd suggest limiting the length we hash/compare is a good compromise. (an example that was tripping this up a lot was something on facebook (or an ad on facebook) was appending 1 char at a time to strings. Always the same char ('A' IIRC). and doing this for 10-60K times. And doing this in a number of variables. This really slowed down the comparisons
Comment 14•6 years ago
|
||
Note that bug 1568923 introduces deduplication for nursery strings during tenuring, which will hopefully dramatically reduce the number of duplicates we see. On the other hand, the initial implementation that Krystal will be landing in that bug doesn't deduplicate ropes, to avoid exactly the sort of blowup cases that this bug is about. (And currently there is no intention to handle that case.)
Comment 15•6 years ago
|
||
What about keeping memory reporting duplicate detection but having it ignore ropes?
It looks like that would still catch some of the problems jesup mentioned in comment 13, though not all. But perhaps the others would be better uncovered via some sort of rope height reporting rather than duplicate detection?
Reporter | ||
Comment 16•6 years ago
|
||
Simply using the first N chars for dups (or first N if it's a rope) would be good enough I think. That shouldn't be that expensive.
Updated•3 years ago
|
Updated•11 months ago
|
Description
•