Bug 115807 – RenderQuote has giant function for language to quotes map

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Darin Adler]

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.

[Darin Adler]

Created attachment 201245 [details]
Patch

[Darin Adler]

I’m seeing what might be a regression test failure and might need to upload a new version of the patch.

[Darin Adler]

Created attachment 201247 [details]
Patch

[Darin Adler]

Had one >= that should have been a >; fixed in the newer patch.

[WebKit Commit Bot]

Comment on attachment 201247 [details]
Patch

Clearing flags on attachment: 201247

Committed r149833: <http://trac.webkit.org/changeset/149833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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:
https://codereview.chromium.org/15093007/

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.

[Darin Adler]

(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.

A fourth problem is that a hash table is not the right data structure for a table that is defined at compile time. Our hash table class optimized for dynamic use at runtime, not a single static table. In other projects I have used gperf to make perfect hash functions for this kind of thing, and JavaScriptCore has something like that too. A simple array with a binary search does the job well. With some helper functions we could make it even easier to do this at other places where this kind of problem arises. Maybe share the code that JavaScriptCore uses.

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.

[Darin Adler]

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.