Bug 223757 - Embiggen maximum HashTable size when not using ANGLE
Summary: Embiggen maximum HashTable size when not using ANGLE
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-25 12:09 PDT by Don Olmstead
Modified: 2021-04-19 09:59 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.23 KB, patch)
2021-03-25 12:30 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2021-03-25 12:09:48 PDT
A HashMap of sh::ShaderVariable is over the limit introduced in https://trac.webkit.org/changeset/274603/webkit

In file included from ../../Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLCommon.cpp:30:
In file included from ../../Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:30:
In file included from ../../Source/WebCore/platform/graphics\GraphicsContextGL.h:32:
In file included from ../../Source/WebCore/platform/graphics/Image.h:30:
In file included from ../../Source/WebCore/platform/graphics/DecodingOptions.h:28:
In file included from ../../Source/WebCore/platform/graphics\IntSize.h:29:
In file included from WTF/Headers\wtf/JSONValues.h:35:
In file included from WTF/Headers\wtf/HashMap.h:25:
WTF/Headers\wtf/HashTable.h:671:9: error: static_assert failed due to requirement 'sizeof(WTF::String) + sizeof(WTF::KeyValuePair<WTF::String, sh::ShaderVariable>) < 250' "Your HashTable types are too big to efficiently move when rehashing.  Consider using UniqueRef instead"
        static_assert(sizeof(Key) + sizeof(Value) < 250, "Your HashTable types are too big to efficiently move when rehashing.  Consider using UniqueRef instead");
        ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

....
                  ^
../../Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLCommon.cpp:301:63: note: in instantiation of member function 'WTF::HashMap<WTF::String, sh::ShaderVariable, WTF::DefaultHash<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTraits<sh::ShaderVariable> >::find' requested here
        const auto& fragmentSymbol = fragmentEntry.varyingMap.find(symbolName);
Comment 1 Don Olmstead 2021-03-25 12:30:00 PDT
Created attachment 424267 [details]
Patch
Comment 2 Alex Christensen 2021-03-25 12:30:58 PDT
Comment on attachment 424267 [details]
Patch

Unfortunately, this is the most straightforward solution.
Comment 3 EWS 2021-03-25 13:20:52 PDT
Committed r275053: <https://commits.webkit.org/r275053>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424267 [details].
Comment 4 Radar WebKit Bug Importer 2021-03-25 13:21:15 PDT
<rdar://problem/75850257>
Comment 5 Sam Weinig 2021-03-28 18:58:03 PDT
Out of interest, what makes using a UniqueRef, as the assert suggests should be done here, not feasible?
Comment 6 Alex Christensen 2021-03-29 09:55:55 PDT
The maps that are hitting this assertion are our use of ShaderSymbolMap.  The value, sh::ShaderVariable, is too big.  Since sh::ShaderVariable isn't fast allocated, we can't use UniqueRef or makeUnique as they stand.  Our solutions were either

1. Make sh::ShaderVariable fast allocated which would require ANGLE to use WTF
2. Use makeUniqueRefWithoutFastMallocCheck
3. This, which doesn't change existing behavior at all.

Given that ports are moving away from !USE(ANGLE), we decided it would be best to just keep the status quo until that happens.
Comment 7 Sam Weinig 2021-03-29 10:08:46 PDT
(In reply to Alex Christensen from comment #6)
> The maps that are hitting this assertion are our use of ShaderSymbolMap. 
> The value, sh::ShaderVariable, is too big.  Since sh::ShaderVariable isn't
> fast allocated, we can't use UniqueRef or makeUnique as they stand.  Our
> solutions were either
> 
> 1. Make sh::ShaderVariable fast allocated which would require ANGLE to use
> WTF
> 2. Use makeUniqueRefWithoutFastMallocCheck
> 3. This, which doesn't change existing behavior at all.
> 
> Given that ports are moving away from !USE(ANGLE), we decided it would be
> best to just keep the status quo until that happens.

I don't understand why makeUniqueRefWithoutFastMallocCheck wouldn't be the right answer here. Having !USE(ANGLE) in the middle of HashTable seems like more invasive change.
Comment 8 Alex Christensen 2021-03-29 10:13:20 PDT
True.  makeUniqueRefWithoutFastMallocCheck would be better.
Comment 9 Alex Christensen 2021-03-29 10:38:18 PDT
The best solution is to remove all the !USE(ANGLE) code, but this is just in the interim.