Bug 239044 - [JSC] Move StructureCache from VM to JSGlobalObject
Summary: [JSC] Move StructureCache from VM to JSGlobalObject
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:
 
Reported: 2022-04-10 03:31 PDT by Yusuke Suzuki
Modified: 2022-04-12 20:55 PDT (History)
8 users (show)

See Also:


Attachments
Patch (23.78 KB, patch)
2022-04-10 03:33 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (24.17 KB, patch)
2022-04-10 05:23 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (25.74 KB, patch)
2022-04-11 12:11 PDT, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2022-04-10 03:31:36 PDT
[JSC] Move StructureCache from VM to JSGlobalObject
Comment 1 Yusuke Suzuki 2022-04-10 03:33:52 PDT
Created attachment 457195 [details]
Patch
Comment 2 Yusuke Suzuki 2022-04-10 05:23:56 PDT
Created attachment 457197 [details]
Patch
Comment 3 Saam Barati 2022-04-11 10:01:12 PDT
Comment on attachment 457197 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3144
> +                structure = globalObject->structureCache().emptyObjectStructureConcurrently(base.getObject(), JSFinalObject::defaultInlineCapacity());

This looks wrong. The runtime code is using the prototype's global object. But we're using the lexical location's global object. Is that mismatch ok?
Comment 4 Yusuke Suzuki 2022-04-11 10:11:27 PDT
Comment on attachment 457197 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3144
>> +                structure = globalObject->structureCache().emptyObjectStructureConcurrently(base.getObject(), JSFinalObject::defaultInlineCapacity());
> 
> This looks wrong. The runtime code is using the prototype's global object. But we're using the lexical location's global object. Is that mismatch ok?

I think Object.create is using its callee's JSGlobalObject (in this case, it is encoded as lexical global object), right?

```
   628 JSC_DEFINE_HOST_FUNCTION(objectConstructorCreate, (JSGlobalObject* globalObject, CallFrame* callFrame))
   629 {
   630     VM& vm = globalObject->vm();
   631     auto scope = DECLARE_THROW_SCOPE(vm);
   632
   633     JSValue proto = callFrame->argument(0);
   634     if (!proto.isObject() && !proto.isNull())
   635         return throwVMTypeError(globalObject, scope, "Object prototype may only be an Object or null."_s);
   636     JSObject* newObject = proto.isObject()
   637         ? constructEmptyObject(globalObject, asObject(proto))
   638         : constructEmptyObject(vm, globalObject->nullPrototypeObjectStructure());
   639     if (callFrame->argument(1).isUndefined())
   640         return JSValue::encode(newObject);
   641     JSObject* properties = callFrame->uncheckedArgument(1).toObject(globalObject);
   642     RETURN_IF_EXCEPTION(scope, { });
   643                                                                                                                                                                                  644     RELEASE_AND_RETURN(scope, JSValue::encode(defineProperties(globalObject, newObject, properties)));
   645 }
```
Comment 5 Justin Michaud 2022-04-11 10:28:27 PDT
Comment on attachment 457197 [details]
Patch

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

> Source/JavaScriptCore/runtime/StructureCache.cpp:-45
> -    PrototypeKey key { makePolyProtoStructure ? nullptr : prototype, executable, inlineCapacity, classInfo, globalObject };

Does this deserve an assert?
Comment 6 Yusuke Suzuki 2022-04-11 12:10:54 PDT
Comment on attachment 457197 [details]
Patch

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

>> Source/JavaScriptCore/runtime/StructureCache.cpp:-45
>> -    PrototypeKey key { makePolyProtoStructure ? nullptr : prototype, executable, inlineCapacity, classInfo, globalObject };
> 
> Does this deserve an assert?

Added.
Comment 7 Yusuke Suzuki 2022-04-11 12:11:57 PDT
Created attachment 457277 [details]
Patch
Comment 8 Saam Barati 2022-04-12 18:28:26 PDT
Comment on attachment 457277 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:10
> +        JSGlobalObject, and (3) simplifies JSGlobalObject::haveABadTime handling.

for (3), you should say in the compiler.
Comment 9 Yusuke Suzuki 2022-04-12 20:52:32 PDT
Comment on attachment 457277 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +        JSGlobalObject, and (3) simplifies JSGlobalObject::haveABadTime handling.
> 
> for (3), you should say in the compiler.

Done.
Comment 10 Yusuke Suzuki 2022-04-12 20:54:46 PDT
Committed r292795 (?): <https://commits.webkit.org/r292795>
Comment 11 Radar WebKit Bug Importer 2022-04-12 20:55:15 PDT
<rdar://problem/91667038>