Bug 184904 – dlopen the bundle's executable before calling -[NSBundle load] since that will also do a bunch of other things we don't need

• Component: WebKit2
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 338627 [details]
WIP

Seems to work at least with Safari. I'm going to do some perf testing both for when it succeeds and when it fails. I want to make sure the second dlopen that NSBundle does inside [NSBundle load] when the dlsym fails is essentially a no-op.

[EWS Watchlist]

Attachment 338627 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:103:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:107:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:110:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 5 in 1 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Doing this speeds up initializeWebProcess by ~60ms on my MBP.

[Saam Barati]

(In reply to Saam Barati from comment #3)
> Doing this speeds up initializeWebProcess by ~60ms on my MBP.

Actually more around ~70ms.

[Saam Barati]

Created attachment 338692 [details]
patch

[Saam Barati]

Comment on attachment 338692 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338692&action=review

> Source/WebKit/ChangeLog:19
> +        This speeds up initializeWebProcess by ~70ms on my MBP.

This speeds up initializeWebProcess on my MBP by close to 2x.

[Geoffrey Garen]

Comment on attachment 338692 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338692&action=review

r=me

> Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:94
> +    if (!dlopenHandle || !initializeFunction) {

I think it's better just to check !initializeFunction here and below, and to scope dlopenHandle to inside the if block.

Logically, we want to fall back if we didn't find the initialize function, regardless of why we didn't find it -- and if we did find it, we don't need to fall back and we don't care about other details.

[Saam Barati]

Comment on attachment 338692 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338692&action=review

>> Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:94
>> +    if (!dlopenHandle || !initializeFunction) {
> 
> I think it's better just to check !initializeFunction here and below, and to scope dlopenHandle to inside the if block.
> 
> Logically, we want to fall back if we didn't find the initialize function, regardless of why we didn't find it -- and if we did find it, we don't need to fall back and we don't care about other details.

Sounds good. The way I do it now just saves us from an extraneous dlsym. But that's probably ok.

[Saam Barati]

Created attachment 338850 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 338850 [details]
patch for landing

Clearing flags on attachment: 338850

Committed r231041: <https://trac.webkit.org/changeset/231041>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39745942>