174919 – Primitive auxiliaries and JSValue auxiliaries should have separate gigacages

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

Summary: Primitive auxiliaries and JSValue auxiliaries should have separate gigacages

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	WebKit Nightly Build
Hardware:	All All

Importance:	P2 Normal
Assignee:	Filip Pizlo

URL:
Keywords:	InRadar

Depends on:	175141
Blocks:	174917 174920 174922 174923 174924
	Show dependency tree / graph

Reported:	2017-07-27 19:38 PDT by Filip Pizlo
Modified:	2017-08-07 17:30 PDT (History)
CC List:	15 users (show)

See Also:

Attachments
it begins (33.59 KB, patch) 2017-08-06 10:40 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
it is written (62.78 KB, patch) 2017-08-06 11:18 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
starting to compile (71.10 KB, patch) 2017-08-06 11:29 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
more (72.87 KB, patch) 2017-08-06 11:43 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
a bit more (112.99 KB, patch) 2017-08-06 12:09 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
the patch (136.56 KB, patch) 2017-08-06 15:29 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
the patch (136.41 KB, patch) 2017-08-07 10:24 PDT, Filip Pizlo	keith_miller: review+	Details \| Formatted Diff \| Diff
patch for landing (136.52 KB, patch) 2017-08-07 14:24 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
patch for landing (136.62 KB, patch) 2017-08-07 14:27 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Filip Pizlo 2017-07-27 19:38:07 PDT

...

Comment 1 Filip Pizlo 2017-08-06 10:40:12 PDT

Created attachment 317367 [details]
it begins

Comment 2 Filip Pizlo 2017-08-06 11:18:17 PDT

Created attachment 317369 [details]
it is written

Maybe?

Comment 3 Filip Pizlo 2017-08-06 11:29:16 PDT

Created attachment 317372 [details]
starting to compile

Comment 4 Filip Pizlo 2017-08-06 11:43:51 PDT

Created attachment 317373 [details]
more

Comment 5 Filip Pizlo 2017-08-06 12:09:50 PDT

Created attachment 317374 [details]
a bit more

Comment 6 Filip Pizlo 2017-08-06 15:29:14 PDT

Created attachment 317385 [details]
the patch

Comment 7 Build Bot 2017-08-06 15:32:12 PDT

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.

Comment 8 Filip Pizlo 2017-08-07 10:24:58 PDT

Created attachment 317432 [details]
the patch

Comment 9 Build Bot 2017-08-07 10:27:05 PDT

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.

Comment 10 Keith Miller 2017-08-07 10:55:36 PDT

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?

Comment 11 Filip Pizlo 2017-08-07 14:24:21 PDT

(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!

Comment 12 Filip Pizlo 2017-08-07 14:24:49 PDT

Created attachment 317465 [details]
patch for landing

Comment 13 Filip Pizlo 2017-08-07 14:27:57 PDT

Created attachment 317467 [details]
patch for landing

Comment 14 Filip Pizlo 2017-08-07 14:30:48 PDT

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

I'll land and ask questions later.

Comment 15 Build Bot 2017-08-07 14:31:54 PDT

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.

Comment 16 Filip Pizlo 2017-08-07 14:32:29 PDT

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

Comment 17 Radar WebKit Bug Importer 2017-08-07 14:32:55 PDT

<rdar://problem/33761863>

Comment 18 Carlos Alberto Lopez Perez 2017-08-07 17:22:44 PDT

(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

Comment 19 Carlos Alberto Lopez Perez 2017-08-07 17:30:53 PDT

(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