Bug 231196

Summary: [Build-time perf] Forward declare JS TypedArrays
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: 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 Flags
Patch
ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing
none
Patch for landing
none
Patch for landing
ews-feeder: commit-queue-
Patch for landing
none
Patch for landing
none
Patch for landing
ews-feeder: commit-queue-
Patch for landing none

Description Jer Noble 2021-10-04 15:52:34 PDT
[Build-time perf] Forward declare JS TypedArrays
Comment 1 Jer Noble 2021-10-04 16:00:25 PDT
Created attachment 440116 [details]
Patch
Comment 2 Tim Horton 2021-10-04 16:12:48 PDT
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
Comment 3 Tim Horton 2021-10-04 16:13:09 PDT
Also might want some JSC person signoff
Comment 4 Jer Noble 2021-10-04 16:23:32 PDT
Created attachment 440118 [details]
Patch for landing
Comment 5 Jer Noble 2021-10-04 16:25:46 PDT
(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 6 Mark Lam 2021-10-04 16:43:40 PDT
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.
Comment 7 Mark Lam 2021-10-04 16:44:47 PDT
(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
Comment 8 Tim Horton 2021-10-04 16:51:20 PDT
(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.
Comment 9 Mark Lam 2021-10-04 16:58:00 PDT
(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.
Comment 10 Jer Noble 2021-10-04 18:07:44 PDT
Created attachment 440133 [details]
Patch for landing
Comment 11 Jer Noble 2021-10-04 21:34:31 PDT
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.
Comment 12 Jer Noble 2021-10-04 21:36:28 PDT
Created attachment 440154 [details]
Patch for landing
Comment 13 Jer Noble 2021-10-04 21:37:25 PDT
Created attachment 440155 [details]
Patch for landing
Comment 14 Jer Noble 2021-10-05 01:14:54 PDT
Created attachment 440180 [details]
Patch for landing
Comment 15 Jer Noble 2021-10-05 09:25:29 PDT
Created attachment 440222 [details]
Patch for landing
Comment 16 Jer Noble 2021-10-05 09:27:58 PDT
Created attachment 440223 [details]
Patch for landing
Comment 17 Jer Noble 2021-10-05 10:53:34 PDT
Created attachment 440235 [details]
Patch for landing
Comment 18 EWS 2021-10-06 00:49:38 PDT
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].
Comment 19 Radar WebKit Bug Importer 2021-10-06 00:50:18 PDT
<rdar://problem/83922505>