Summary: | [Build-time perf] Forward declare JS TypedArrays | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | annulen, calvaris, cdumez, cgarcia, changseok, dino, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gustavo, gyuyoung.kim, hta, jiewen_tan, kangil.han, keith_miller, kondapallykalyan, macpherson, mark.lam, menard, msaboff, philipj, pnormand, ryuan.choi, saam, sergio, thorton, tommyw, tzagallo, vjaquez, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Jer Noble
2021-10-04 15:52:34 PDT
Created attachment 440116 [details]
Patch
Comment on attachment 440116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440116&action=review Seems a bit red, might need some iteration > Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeys.cpp:32 > +#include <JavaScriptCore/Uint8Array.h> > #include "HTMLMediaElement.h" I think you've got this in the wrong place Also might want some JSC person signoff Created attachment 440118 [details]
Patch for landing
(In reply to Tim Horton from comment #2) > Comment on attachment 440116 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=440116&action=review > > Seems a bit red, might need some iteration > > > Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeys.cpp:32 > > +#include <JavaScriptCore/Uint8Array.h> > > #include "HTMLMediaElement.h" > > I think you've got this in the wrong place Whoops, you're right. Not sure why the style bot is fine with this. Comment on attachment 440116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440116&action=review > Source/JavaScriptCore/ChangeLog:8 > + Add a new file, Forward.h, containing forward declarations of commonly used JavaScriptCore types. I believe the code in this Forward.h came from TypedArrayAdaptors.h. 1. Calling this file Forward.h seems awfully generic. I can see people being incentivized to pile all kinds of things in there (which is not good). Let's call it TypedArrayAdaptorForwardDeclarations.h or TypedArrayAdaptorForward.h or something more specific like that. 2. Rather than declaring these in 2 places, let's make TypedArrayAdaptors.h #include TypedArrayAdaptorForwardDeclarations.h instead. 3. I also see redundant declarations in TypedArrays.h. I think those can be removed since it #includes TypedArrayAdaptors.h. (In reply to Mark Lam from comment #6) > 1. Calling this file Forward.h seems awfully generic. I can see people > being incentivized to pile all kinds of things in there (which is not good). > Let's call it TypedArrayAdaptorForwardDeclarations.h or > TypedArrayAdaptorForward.h or something more specific like that. Or TypedArrayAdaptorsForward.h (In reply to Mark Lam from comment #6) > Comment on attachment 440116 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=440116&action=review > > > Source/JavaScriptCore/ChangeLog:8 > > + Add a new file, Forward.h, containing forward declarations of commonly used JavaScriptCore types. > > I believe the code in this Forward.h came from TypedArrayAdaptors.h. > 1. Calling this file Forward.h seems awfully generic. I can see people > being incentivized to pile all kinds of things in there (which is not good). Would it be so bad? That is the approach in WTF. (In reply to Tim Horton from comment #8) > (In reply to Mark Lam from comment #6) > > Comment on attachment 440116 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=440116&action=review > > > > > Source/JavaScriptCore/ChangeLog:8 > > > + Add a new file, Forward.h, containing forward declarations of commonly used JavaScriptCore types. > > > > I believe the code in this Forward.h came from TypedArrayAdaptors.h. > > 1. Calling this file Forward.h seems awfully generic. I can see people > > being incentivized to pile all kinds of things in there (which is not good). > > Would it be so bad? That is the approach in WTF. Taken in light of these: > 2. Rather than declaring these in 2 places, let's make TypedArrayAdaptors.h #include TypedArrayAdaptorForwardDeclarations.h instead. > 3. I also see redundant declarations in TypedArrays.h. I think those can be removed since it #includes TypedArrayAdaptors.h. ... yes, I would rather these not #include a generic Forward.h that is the dumping ground for everything and anything. Created attachment 440133 [details]
Patch for landing
Requiring clients who just want to get a forward declaration of Uint8Array to #include <JavaScriptCore/TypeArrayAdapterForwardDeclarations.h> would be a usability nightmare, but we could square that circle by just #including "TypedArrayAdaptersForwardDeclarations.h" inside Forward.h. Then Forward.h could become a "dumping ground" (a.k.a., a nice way to get declarations of common JSC types) without that impacting TypedArrayAdapters.h and would result in zero duplicate type declarations. Created attachment 440154 [details]
Patch for landing
Created attachment 440155 [details]
Patch for landing
Created attachment 440180 [details]
Patch for landing
Created attachment 440222 [details]
Patch for landing
Created attachment 440223 [details]
Patch for landing
Created attachment 440235 [details]
Patch for landing
Committed r283605 (242557@main): <https://commits.webkit.org/242557@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440235 [details]. |