Bug 193713 - [JSC] NativeErrorConstructor should not have own IsoSubspace
Summary: [JSC] NativeErrorConstructor should not have own IsoSubspace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (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-23 03:24 PST by Yusuke Suzuki
Modified: 2019-01-28 17:53 PST (History)
10 users (show)

See Also:


Attachments
Patch (48.61 KB, patch)
2019-01-25 17:10 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (48.75 KB, patch)
2019-01-25 17:15 PST, Yusuke Suzuki
sbarati: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Patch (49.31 KB, patch)
2019-01-25 17:30 PST, Yusuke Suzuki
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.87 MB, application/zip)
2019-01-25 18:38 PST, EWS Watchlist
no flags Details
Patch for landing (50.67 KB, patch)
2019-01-25 19:13 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (50.94 KB, patch)
2019-01-25 19:24 PST, Yusuke Suzuki
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (292.01 KB, application/zip)
2019-01-25 20:17 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (1.69 MB, application/zip)
2019-01-25 20:28 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (624.84 KB, application/zip)
2019-01-25 20:51 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews204 for win-future (1.87 MB, application/zip)
2019-01-25 21:02 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (675.10 KB, application/zip)
2019-01-25 21:14 PST, EWS Watchlist
no flags Details
Patch for landing (53.26 KB, patch)
2019-01-25 22:30 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (54.56 KB, patch)
2019-01-25 23:34 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-23 03:24:25 PST
It's too costly for NativeErrorConstructor. We should take the similar implementation to our TypedArrayViewConstructors.
Comment 1 Yusuke Suzuki 2019-01-25 17:10:12 PST
Created attachment 360196 [details]
Patch
Comment 2 Yusuke Suzuki 2019-01-25 17:15:00 PST
Created attachment 360198 [details]
Patch
Comment 3 Yusuke Suzuki 2019-01-25 17:19:10 PST
Comment on attachment 360198 [details]
Patch

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

> Source/JavaScriptCore/runtime/NativeErrorConstructor.cpp:73
> +// Instantiate JSGenericArrayBufferConstructors.

Oops, this comment is meaningless and wrong. Removed.
Comment 4 Saam Barati 2019-01-25 17:20:48 PST
Comment on attachment 360198 [details]
Patch

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

> Source/JavaScriptCore/runtime/NativeErrorConstructor.cpp:73
> +// Instantiate JSGenericArrayBufferConstructors.

Wrong comment.
Comment 5 Saam Barati 2019-01-25 17:24:29 PST
You have some build issues.
Comment 6 Yusuke Suzuki 2019-01-25 17:28:42 PST
Oops, yeah, I need to make ErrorType.h Private instead of Project in Xcode.
Comment 7 Yusuke Suzuki 2019-01-25 17:30:59 PST
Created attachment 360199 [details]
Patch

EWS
Comment 8 Mark Lam 2019-01-25 17:37:28 PST
Comment on attachment 360198 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        threads, this is OK. While TypeError constructor is eagerly allocated because it is touched from our builtin JS as @TypeError, we should
> +        offer some function instead of exposing TypeError constructor in the future, and remove this @TypeError reference. This change removes

Add a bug and FIXME for "offer some function instead of exposing TypeError constructor in the future, and remove this @TypeError reference"?

> Source/JavaScriptCore/runtime/ErrorType.cpp:39
> +static ASCIILiteral errorTypeNames[] = {
> +    "Error"_s,
> +    "EvalError"_s,
> +    "RangeError"_s,
> +    "ReferenceError"_s,
> +    "SyntaxError"_s,
> +    "TypeError"_s,
> +    "URIError"_s,
> +};

Suggestion: make this a static const char* const errorTypeNames[] and move it inside the errorTypeName() function below?
Comment 9 Yusuke Suzuki 2019-01-25 17:49:17 PST
Comment on attachment 360198 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:11
>> +        offer some function instead of exposing TypeError constructor in the future, and remove this @TypeError reference. This change removes
> 
> Add a bug and FIXME for "offer some function instead of exposing TypeError constructor in the future, and remove this @TypeError reference"?

Cool. Added https://bugs.webkit.org/show_bug.cgi?id=193858.

>> Source/JavaScriptCore/runtime/ErrorType.cpp:39
>> +};
> 
> Suggestion: make this a static const char* const errorTypeNames[] and move it inside the errorTypeName() function below?

Discussed with Mark, we should move this inside the errorTypeName function with `static const ASCIILiteral const errorTypeNames[]`.
Comment 10 Yusuke Suzuki 2019-01-25 17:59:50 PST
Comment on attachment 360198 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/ErrorType.cpp:39
>>> +};
>> 
>> Suggestion: make this a static const char* const errorTypeNames[] and move it inside the errorTypeName() function below?
> 
> Discussed with Mark, we should move this inside the errorTypeName function with `static const ASCIILiteral const errorTypeNames[]`.

static const ASCIILiteral errorTypeNames[], and make field of ASCIILiteral `const char* const`.
Comment 11 EWS Watchlist 2019-01-25 18:38:05 PST
Comment on attachment 360199 [details]
Patch

Attachment 360199 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10896019

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/specifier-error.html
http/tests/security/cross-frame-access-put.html
fast/frames/frame-window-as-callback.html
js/dom/function-names.html
Comment 12 EWS Watchlist 2019-01-25 18:38:07 PST
Created attachment 360206 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 13 Yusuke Suzuki 2019-01-25 18:41:10 PST
Comment on attachment 360199 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:406
> +    NativeErrorPrototype* prototype = NativeErrorPrototype::create(init.vm, m_nativeErrorPrototypeStructure.get(), errorTypeName(errorType));
> +    init.setPrototype(prototype);
> +    Structure* structure = ErrorInstance::createStructure(init.vm, this, prototype);
> +    init.setStructure(structure);
> +    init.setConstructor(NativeErrorConstructor<errorType>::create(init.vm, NativeErrorConstructor<errorType>::createStructure(init.vm, this, prototype), prototype));
> +    prototype->putDirect(init.vm, init.vm.propertyNames->constructor, init.constructor, static_cast<unsigned>(PropertyAttribute::DontEnum));

Oops, I fixed NativeErrorConstructor<errorType>::createStructure(init.vm, this, m_errorConstructor.get()). I aligned this code to LazyClassStructure's use.

init.setPrototype(NativeErrorPrototype::create(init.vm, NativeErrorPrototype::createStructure(init.vm, this, m_errorPrototype.get()), errorTypeName(errorType)));
init.setStructure(ErrorInstance::createStructure(init.vm, this, init.prototype));
init.setConstructor(NativeErrorConstructor<errorType>::create(init.vm, NativeErrorConstructor<errorType>::createStructure(init.vm, this, m_errorConstructor.get()), jsCast<NativeErrorPrototype*>(init.prototype)));

init.setConstructor automatically perform init.prototype->putDirectWithoutTransition("constructor", constructor ...) thing.
Comment 14 Yusuke Suzuki 2019-01-25 19:13:22 PST
Created attachment 360212 [details]
Patch for landing
Comment 15 Yusuke Suzuki 2019-01-25 19:24:36 PST
Created attachment 360213 [details]
Patch for landing
Comment 16 EWS Watchlist 2019-01-25 20:17:42 PST
Comment on attachment 360213 [details]
Patch for landing

Attachment 360213 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10897233

Number of test failures exceeded the failure limit.
Comment 17 EWS Watchlist 2019-01-25 20:17:43 PST
Created attachment 360216 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 18 EWS Watchlist 2019-01-25 20:28:40 PST
Comment on attachment 360213 [details]
Patch for landing

Attachment 360213 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10897197

Number of test failures exceeded the failure limit.
Comment 19 EWS Watchlist 2019-01-25 20:28:42 PST
Created attachment 360219 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 20 EWS Watchlist 2019-01-25 20:33:55 PST
Comment on attachment 360198 [details]
Patch

Attachment 360198 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10896540

New failing tests:
stress/try-get-by-id-should-spill-registers-dfg.js.dfg-eager
stress/try-get-by-id-should-spill-registers-dfg.js.bytecode-cache
stress/try-get-by-id-should-spill-registers-dfg.js.no-cjit-validate-phases
ChakraCore.yaml/ChakraCore/test/Lib/error.js.default
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-eager
ChakraCore.yaml/ChakraCore/test/Error/ErrorCtorProps.js.default
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-no-cjit-validate-sampling-profiler
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-eager-no-cjit
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-no-cjit-small-pool
stress/try-get-by-id-should-spill-registers-dfg.js.no-llint
stress/try-get-by-id-should-spill-registers-dfg.js.dfg-maximal-flush-validate-no-cjit
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-eager-no-cjit-b3o1
stress/try-get-by-id-should-spill-registers-dfg.js.dfg-eager-no-cjit-validate
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-no-cjit-no-put-stack-validate
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-no-cjit-b3o1
stress/try-get-by-id-should-spill-registers-dfg.js.no-cjit-collect-continuously
stress/try-get-by-id-should-spill-registers-dfg.js.no-ftl
stress/try-get-by-id-should-spill-registers-dfg.js.default
ChakraCore.yaml/ChakraCore/test/Lib/tostring.js.default
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-no-cjit-no-inline-validate
apiTests
Comment 21 EWS Watchlist 2019-01-25 20:51:32 PST
Comment on attachment 360213 [details]
Patch for landing

Attachment 360213 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10897308

Number of test failures exceeded the failure limit.
Comment 22 EWS Watchlist 2019-01-25 20:51:34 PST
Created attachment 360220 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 23 EWS Watchlist 2019-01-25 21:02:51 PST
Comment on attachment 360213 [details]
Patch for landing

Attachment 360213 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10897395

Number of test failures exceeded the failure limit.
Comment 24 EWS Watchlist 2019-01-25 21:02:54 PST
Created attachment 360221 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 25 EWS Watchlist 2019-01-25 21:14:27 PST
Comment on attachment 360213 [details]
Patch for landing

Attachment 360213 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10897609

Number of test failures exceeded the failure limit.
Comment 26 EWS Watchlist 2019-01-25 21:14:35 PST
Created attachment 360222 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 27 Yusuke Suzuki 2019-01-25 22:30:59 PST
Created attachment 360224 [details]
Patch for landing
Comment 28 Yusuke Suzuki 2019-01-25 23:34:57 PST
Created attachment 360230 [details]
Patch for landing
Comment 29 Yusuke Suzuki 2019-01-26 01:07:30 PST
Committed r240543: <https://trac.webkit.org/changeset/240543>
Comment 30 Radar WebKit Bug Importer 2019-01-26 01:08:31 PST
<rdar://problem/47572897>
Comment 31 Ryan Haddad 2019-01-27 22:21:45 PST
The test added with this change is crashing on the debug JSC bot:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2080
Comment 32 Yusuke Suzuki 2019-01-28 17:53:28 PST
Committed r240629: <https://trac.webkit.org/changeset/240629>