Anders pointed out that RenderQuote has a colossal function that creates a map from language to quotes. It doesn’t need that. Also some other style problems there worth fixing.
Created attachment 201245 [details]
I’m seeing what might be a regression test failure and might need to upload a new version of the patch.
Created attachment 201247 [details]
Had one >= that should have been a >; fixed in the newer patch.
Comment on attachment 201247 [details]
Clearing flags on attachment: 201247
Committed r149833: <http://trac.webkit.org/changeset/149833>
All reviewed patches have been landed. Closing bug.
I think the root problem here is HashTable::set.
Each of these set() calls appears to expand to 1.52k of binary size!
See more discussion here:
My template-fu is not strong enough to understand what HashTable.h is doing which is causing this binary size explosion with each :set() call.
(In reply to comment #7)
> I think the root problem here is HashTable::set.
I’m not sure; that’s a significant part of the problem, but I would not call it “the root problem”. One of the best features of HashTable is that the code gets inlined where it needs to be fast.
> Each of these set() calls appears to expand to 1.52k of binary size!
We do expect the calls to be fairly large, because our hash table code is designed to be fully inlined by default.
I see four problems here:
This function’s original code makes the mistake of compiling lots of separate inlined calls to fill out the hash table rather than using a loop. That’s the primary problem. Generating a lot of code instead of a loop. Fixing that would be the minimal fix. I consider that “the root problem”. It’s just not a good approach to generate lots of function calls instead of a table and a loop. There’s no reason that loop needs to be unrolled.
The second problem is that the code to set a hash table entry is big, perhaps bigger than it needs to be. You called that “the root problem”, and it is an area with room for improvement but not really the key here in my opinion.
A third possibly distinct problem is that this code uses the inlined version, but it should instead use a function because it doesn’t benefit from inlining. We might want to look at different inlining for some hash table code in general, or a way to avoid the inlining without hand-writing functions each time.
Anders’s comment to me about this function was that we don’t need this on-time code to be inlined at all. Then Anders and I decided to take it further and not use a hash table at all, because we don’t think a hash table is the right tool for this job.
Elliot mentioned in the Blink code review that this new code is ugly. I think it’s worth considering how to package this for reuse as a way to make it prettier since there are similar problems to be solved elsewhere. That’s probably more practical than turning HashMap into a tool suitable for this use.