Bug 174919 – Primitive auxiliaries and JSValue auxiliaries should have separate gigacages

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

...

[Filip Pizlo]

Created attachment 317367 [details]
it begins

[Filip Pizlo]

Created attachment 317369 [details]
it is written

Maybe?

[Filip Pizlo]

Created attachment 317372 [details]
starting to compile

[Filip Pizlo]

Created attachment 317373 [details]
more

[Filip Pizlo]

Created attachment 317374 [details]
a bit more

[Filip Pizlo]

Created attachment 317385 [details]
the patch

[Build Bot]

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


ERROR: Source/WTF/wtf/Gigacage.h:76:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:79:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:80:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:88:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:93:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:94:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:119:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:190:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:37:  g_primitiveGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:38:  g_jsValueGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/GPUBufferMetal.mm:52:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BInline.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/bmalloc/bmalloc/Cache.cpp:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 13 in 55 files


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

[Filip Pizlo]

Created attachment 317432 [details]
the patch

[Build Bot]

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


ERROR: Source/WTF/wtf/Gigacage.h:76:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:79:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:80:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:88:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:93:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:94:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:119:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:190:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:37:  g_primitiveGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:38:  g_jsValueGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/GPUBufferMetal.mm:52:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BInline.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/bmalloc/bmalloc/Cache.cpp:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 13 in 55 files


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

[Keith Miller]

Comment on attachment 317432 [details]
the patch

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

r=me.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:11623
> +        if (kind == Gigacage::Primitive) {
> +            if (vm().primitiveGigacageEnabled().isStillValid())
> +                m_graph.watchpoints().addLazily(vm().primitiveGigacageEnabled());
> +            else
> +                return ptr;

Just to double check. We don't need this for the JSValue Gigacage because it cannot be disabled?

[Filip Pizlo]

(In reply to Keith Miller from comment #10)
> Comment on attachment 317432 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317432&action=review
> 
> r=me.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:11623
> > +        if (kind == Gigacage::Primitive) {
> > +            if (vm().primitiveGigacageEnabled().isStillValid())
> > +                m_graph.watchpoints().addLazily(vm().primitiveGigacageEnabled());
> > +            else
> > +                return ptr;
> 
> Just to double check. We don't need this for the JSValue Gigacage because it
> cannot be disabled?

Yup!

[Filip Pizlo]

Created attachment 317465 [details]
patch for landing

[Filip Pizlo]

Created attachment 317467 [details]
patch for landing

[Filip Pizlo]

I think I fixed the Windows build issues, but now Windows EWS has gone away.

I'll land and ask questions later.

[Build Bot]

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


ERROR: Source/WTF/wtf/Gigacage.h:76:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:79:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:80:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:88:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:93:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:94:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:119:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:190:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:37:  g_primitiveGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:38:  g_jsValueGigacageBasePtr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/Gigacage.cpp:36:  g_gigacageBasePtr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/GPUBufferMetal.mm:52:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BInline.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/bmalloc/bmalloc/Cache.cpp:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 14 in 55 files


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

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/220352/webkit

[Radar WebKit Bug Importer]

<rdar://problem/33761863>

[Carlos Alberto Lopez Perez]

(In reply to Filip Pizlo from comment #16)
> Landed in https://trac.webkit.org/changeset/220352/webkit

It seems this has caused around 3600 new failures on the Linux JSC tests.

* GTK+ test run at r220351 (jscore-test step green) https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/2495
* GTK+ test run at r220352 (3645 JSC tests failed) https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/2496

* WPE test run at r220351 (jscore-test step green) https://build.webkit.org/builders/WPE%20Linux%2064-bit%20Release%20%28Tests%29/builds/1922
* WPE test run at r220352 (3623 JSC tests failed) https://build.webkit.org/builders/WPE%20Linux%2064-bit%20Release%20%28Tests%29/builds/1923

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #18)
> (In reply to Filip Pizlo from comment #16)
> > Landed in https://trac.webkit.org/changeset/220352/webkit
> 
> It seems this has caused around 3600 new failures on the Linux JSC tests.
> 
> * GTK+ test run at r220351 (jscore-test step green)
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/2495
> * GTK+ test run at r220352 (3645 JSC tests failed)
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/2496
> 
> * WPE test run at r220351 (jscore-test step green)
> https://build.webkit.org/builders/WPE%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/1922
> * WPE test run at r220352 (3623 JSC tests failed)
> https://build.webkit.org/builders/WPE%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/1923

And r220368 fixed it? WPE bot is back to green after it: https://build.webkit.org/builders/WPE%20Linux%2064-bit%20Release%20%28Tests%29/builds/1928