Bug 231196

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:

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

Jer Noble

Reported 2021-10-04 15:52:34 PDT

[Build-time perf] Forward declare JS TypedArrays

Attachments
Patch (34.69 KB, patch) 2021-10-04 16:00 PDT, Jer Noble	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch for landing (36.49 KB, patch) 2021-10-04 16:23 PDT, Jer Noble	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch for landing (38.20 KB, patch) 2021-10-04 18:07 PDT, Jer Noble	no flags	Details Formatted Diff Diff
Patch for landing (51.09 KB, patch) 2021-10-04 21:36 PDT, Jer Noble	no flags	Details Formatted Diff Diff
Patch for landing (51.16 KB, patch) 2021-10-04 21:37 PDT, Jer Noble	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch for landing (52.79 KB, patch) 2021-10-05 01:14 PDT, Jer Noble	no flags	Details Formatted Diff Diff
Patch for landing (52.68 KB, patch) 2021-10-05 09:25 PDT, Jer Noble	no flags	Details Formatted Diff Diff
Patch for landing (52.68 KB, patch) 2021-10-05 09:27 PDT, Jer Noble	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch for landing (53.81 KB, patch) 2021-10-05 10:53 PDT, Jer Noble	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Jer Noble

Comment 1 2021-10-04 16:00:25 PDT

Created attachment 440116 [details] Patch

Tim Horton

Comment 2 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

Tim Horton

Comment 3 2021-10-04 16:13:09 PDT

Also might want some JSC person signoff

Jer Noble

Comment 4 2021-10-04 16:23:32 PDT

Created attachment 440118 [details] Patch for landing

Jer Noble

Comment 5 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.

Mark Lam

Comment 6 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.

Mark Lam

Comment 7 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

Tim Horton

Comment 8 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.

Mark Lam

Comment 9 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.

Jer Noble

Comment 10 2021-10-04 18:07:44 PDT

Created attachment 440133 [details] Patch for landing

Jer Noble

Comment 11 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.

Jer Noble

Comment 12 2021-10-04 21:36:28 PDT

Created attachment 440154 [details] Patch for landing

Jer Noble

Comment 13 2021-10-04 21:37:25 PDT

Created attachment 440155 [details] Patch for landing

Jer Noble

Comment 14 2021-10-05 01:14:54 PDT

Created attachment 440180 [details] Patch for landing

Jer Noble

Comment 15 2021-10-05 09:25:29 PDT

Created attachment 440222 [details] Patch for landing

Jer Noble

Comment 16 2021-10-05 09:27:58 PDT

Created attachment 440223 [details] Patch for landing

Jer Noble

Comment 17 2021-10-05 10:53:34 PDT

Created attachment 440235 [details] Patch for landing

EWS

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

Radar WebKit Bug Importer

Comment 19 2021-10-06 00:50:18 PDT

<rdar://problem/83922505>

Note You need to log in before you can comment on or make changes to this bug.