Bug 231196 – [Build-time perf] Forward declare JS TypedArrays

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[Build-time perf] Forward declare JS TypedArrays

[Jer Noble]

Created attachment 440116 [details]
Patch

[Tim Horton]

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

[Tim Horton]

Also might want some JSC person signoff

[Jer Noble]

Created attachment 440118 [details]
Patch for landing

[Jer Noble]

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

[Mark Lam]

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.

[Mark Lam]

(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

[Tim Horton]

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

[Mark Lam]

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

[Jer Noble]

Created attachment 440133 [details]
Patch for landing

[Jer Noble]

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.

[Jer Noble]

Created attachment 440154 [details]
Patch for landing

[Jer Noble]

Created attachment 440155 [details]
Patch for landing

[Jer Noble]

Created attachment 440180 [details]
Patch for landing

[Jer Noble]

Created attachment 440222 [details]
Patch for landing

[Jer Noble]

Created attachment 440223 [details]
Patch for landing

[Jer Noble]

Created attachment 440235 [details]
Patch for landing

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/83922505>