RESOLVED FIXED Bug 174919
Primitive auxiliaries and JSValue auxiliaries should have separate gigacages
https://bugs.webkit.org/show_bug.cgi?id=174919
Summary Primitive auxiliaries and JSValue auxiliaries should have separate gigacages
Filip Pizlo
Reported 2017-07-27 19:38:07 PDT
...
Attachments
it begins (33.59 KB, patch)
2017-08-06 10:40 PDT, Filip Pizlo
no flags
it is written (62.78 KB, patch)
2017-08-06 11:18 PDT, Filip Pizlo
no flags
starting to compile (71.10 KB, patch)
2017-08-06 11:29 PDT, Filip Pizlo
no flags
more (72.87 KB, patch)
2017-08-06 11:43 PDT, Filip Pizlo
no flags
a bit more (112.99 KB, patch)
2017-08-06 12:09 PDT, Filip Pizlo
no flags
the patch (136.56 KB, patch)
2017-08-06 15:29 PDT, Filip Pizlo
no flags
the patch (136.41 KB, patch)
2017-08-07 10:24 PDT, Filip Pizlo
keith_miller: review+
patch for landing (136.52 KB, patch)
2017-08-07 14:24 PDT, Filip Pizlo
no flags
patch for landing (136.62 KB, patch)
2017-08-07 14:27 PDT, Filip Pizlo
no flags
Filip Pizlo
Comment 1 2017-08-06 10:40:12 PDT
Created attachment 317367 [details] it begins
Filip Pizlo
Comment 2 2017-08-06 11:18:17 PDT
Created attachment 317369 [details] it is written Maybe?
Filip Pizlo
Comment 3 2017-08-06 11:29:16 PDT
Created attachment 317372 [details] starting to compile
Filip Pizlo
Comment 4 2017-08-06 11:43:51 PDT
Filip Pizlo
Comment 5 2017-08-06 12:09:50 PDT
Created attachment 317374 [details] a bit more
Filip Pizlo
Comment 6 2017-08-06 15:29:14 PDT
Created attachment 317385 [details] the patch
Build Bot
Comment 7 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.
Filip Pizlo
Comment 8 2017-08-07 10:24:58 PDT
Created attachment 317432 [details] the patch
Build Bot
Comment 9 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.
Keith Miller
Comment 10 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?
Filip Pizlo
Comment 11 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!
Filip Pizlo
Comment 12 2017-08-07 14:24:49 PDT
Created attachment 317465 [details] patch for landing
Filip Pizlo
Comment 13 2017-08-07 14:27:57 PDT
Created attachment 317467 [details] patch for landing
Filip Pizlo
Comment 14 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.
Build Bot
Comment 15 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.
Filip Pizlo
Comment 16 2017-08-07 14:32:29 PDT
Radar WebKit Bug Importer
Comment 17 2017-08-07 14:32:55 PDT
Carlos Alberto Lopez Perez
Comment 18 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
Carlos Alberto Lopez Perez
Comment 19 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
Note You need to log in before you can comment on or make changes to this bug.