Bug 193646 - [JSC] Lazily initialize JSModuleLoader
Summary: [JSC] Lazily initialize JSModuleLoader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 193606
  Show dependency treegraph
 
Reported: 2019-01-20 23:17 PST by Yusuke Suzuki
Modified: 2019-01-21 16:28 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.27 KB, patch)
2019-01-20 23:18 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.61 KB, patch)
2019-01-20 23:38 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (4.38 KB, patch)
2019-01-21 13:54 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-01-20 23:17:04 PST
[JSC] Lazily initialize JSModuleLoader
Comment 1 Yusuke Suzuki 2019-01-20 23:18:43 PST
Created attachment 359672 [details]
Patch
Comment 2 Yusuke Suzuki 2019-01-20 23:38:35 PST
Created attachment 359673 [details]
Patch
Comment 3 Keith Miller 2019-01-21 08:45:22 PST
Comment on attachment 359673 [details]
Patch

r=me.
Comment 4 Saam Barati 2019-01-21 10:42:54 PST
Comment on attachment 359673 [details]
Patch

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

> Source/JavaScriptCore/runtime/HashMapImpl.h:206
> +    static HashMapBuffer* create(ExecState* nullOrExecForOOM, VM& vm, JSCell*, uint32_t capacity)

not a fan of this, why not globalExec?

> Source/JavaScriptCore/runtime/JSModuleLoader.cpp:109
> +    JSMap* map = JSMap::create(nullptr, vm, globalObject->mapStructure());

why not globalExec?
Comment 5 Saam Barati 2019-01-21 10:44:00 PST
Comment on attachment 359673 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSModuleLoader.cpp:109
>> +    JSMap* map = JSMap::create(nullptr, vm, globalObject->mapStructure());
> 
> why not globalExec?

If you don't want an exception to be thrown, I'd use globalExec, and then releaseAssertNoException here instead of passing in nullptr. I think the abstraction is backwards this way, but not in the way I propose.
Comment 6 Yusuke Suzuki 2019-01-21 13:53:23 PST
Comment on attachment 359673 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/HashMapImpl.h:206
>> +    static HashMapBuffer* create(ExecState* nullOrExecForOOM, VM& vm, JSCell*, uint32_t capacity)
> 
> not a fan of this, why not globalExec?

Personally, rather, I would like to drop this ExecState* parameter for simple `JSMap::create()` / `JSModuleLoader::create` cases because it allocates fixed amount of memory which is not so large.
But in the meantime, we still have this parameter. So, using globalExec makes sense. Fixed.

>>> Source/JavaScriptCore/runtime/JSModuleLoader.cpp:109
>>> +    JSMap* map = JSMap::create(nullptr, vm, globalObject->mapStructure());
>> 
>> why not globalExec?
> 
> If you don't want an exception to be thrown, I'd use globalExec, and then releaseAssertNoException here instead of passing in nullptr. I think the abstraction is backwards this way, but not in the way I propose.

Fixed.
Comment 7 Yusuke Suzuki 2019-01-21 13:54:20 PST
Created attachment 359709 [details]
Patch
Comment 8 Yusuke Suzuki 2019-01-21 14:01:22 PST
Committed r240242: <https://trac.webkit.org/changeset/240242>
Comment 9 Radar WebKit Bug Importer 2019-01-21 14:02:28 PST
<rdar://problem/47432806>