Bug 53219 – Convert markstack to a slot visitor API

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Convert markstack to a slot visitor API

[Oliver Hunt]

Created attachment 80382 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1

Source/JavaScriptCore/runtime/JSArray.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 114 files


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

[Early Warning System Bot]

Attachment 80382 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7651064

[Build Bot]

Attachment 80382 [details] did not build on win:
Build output: http://queues.webkit.org/results/7593365

[Oliver Hunt]

Created attachment 80390 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1

Source/JavaScriptCore/runtime/JSArray.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 114 files


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

[Early Warning System Bot]

Attachment 80390 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7532385

[WebKit Review Bot]

Attachment 80390 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7561392

[WebKit Review Bot]

Attachment 80390 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7603393

[Geoffrey Garen]

-        ALWAYS_INLINE void append(JSValue);
-        void append(JSCell*);
+        void append(JSValue*);
+        void append(JSCell**);

I think we want these APIs to be 

deprecatedAppend(JSValue*);
template <typename T> deprecatedAppend(DeprecatedPtr<T>);
template <typename T> append(WriteBarrier<T>);

We rely on MarkStack's interface, and the help of the compiler, to suss out all the objects that aren't yet moving-safe. I think it's worth it to limit the surface area here, not allowing any JSCell**, and to call out the deprecated interfaces explicitly.

[Geoffrey Garen]

+    template <> class WriteBarrierBase<Unknown> {

What's the goal of this separate base class?

+    protected:
+        EncodedJSValue m_value;

Can we store a JSValue instead of an EncodedJSValue?


+++ b/Source/JavaScriptCore/runtime/JSValue.h
@@ -23,6 +23,8 @@
 #ifndef JSValue_h
 #define JSValue_h
 
+#include "WriteBarrier.h"

It's confusing that half the WriteBarrier and DeprecatedPtr class is in WriteBarrier.h, and half is in JSValue.h. Can we put them in the same file, and have WriterBarrier.h #include JSValue.h? JSValue declares two constructor declarations that depend on a template declaration, but I think you can just add that those declarations stand-alone.

[Oliver Hunt]

(In reply to comment #11)
> +    template <> class WriteBarrierBase<Unknown> {
> 
> What's the goal of this separate base class?
> 
> +    protected:
> +        EncodedJSValue m_value;
> 
> Can we store a JSValue instead of an EncodedJSValue?
Nope.

> 
> 
> +++ b/Source/JavaScriptCore/runtime/JSValue.h
> @@ -23,6 +23,8 @@
>  #ifndef JSValue_h
>  #define JSValue_h
> 
> +#include "WriteBarrier.h"
> 
> It's confusing that half the WriteBarrier and DeprecatedPtr class is in WriteBarrier.h, and half is in JSValue.h. Can we put them in the same file, and have WriterBarrier.h #include JSValue.h? JSValue declares two constructor declarations that depend on a template declaration, but I think you can just add that those declarations stand-alone.

I will give that a shot

[Geoffrey Garen]

> > Can we store a JSValue instead of an EncodedJSValue?
> Nope.

Can you explain why we need to store an EncodedJSValue?

[Oliver Hunt]

(In reply to comment #13)
> > > Can we store a JSValue instead of an EncodedJSValue?
> > Nope.
> 
> Can you explain why we need to store an EncodedJSValue?

Because using a JSValue creates an implicit constructor in WriteBarrierBase.

We need WriteBarrierBase to have no constructor so that we can put it in the JSObject data union.  That's also why there's the WriteBarrierBase and WriteBarrier distinction - WriteBarrier is the easier one to write, and has helpful constructors, but we need a common base class so that JSObject is sane.

[Oliver Hunt]

Created attachment 80482 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1

Source/JavaScriptCore/runtime/JSArray.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 121 files


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

[Early Warning System Bot]

Attachment 80482 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7552349

[Oliver Hunt]

Created attachment 80503 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1

Source/JavaScriptCore/runtime/JSArray.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 124 files


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

[Geoffrey Garen]

Comment on attachment 80503 [details]
Patch

r=me

[Oliver Hunt]

Committed r77006: <http://trac.webkit.org/changeset/77006>

[Ryosuke Niwa]

This patch broke hundreds of layout tests on Windows 7:
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r77004%20(8776)/results.html
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r77006%20(8777)/results.html

I'm sorry but I had to roll it out in http://trac.webkit.org/changeset/77044.

[Oliver Hunt]

(In reply to comment #22)
> This patch broke hundreds of layout tests on Windows 7:
> http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r77004%20(8776)/results.html
> http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r77006%20(8777)/results.html
> 
> I'm sorry but I had to roll it out in http://trac.webkit.org/changeset/77044.

Rolled back in with 32bit fix in r77098

[Csaba Osztrogonác]

Reopen, because it was rolled out by http://trac.webkit.org/changeset/77113

Qt build was broken for ~6 hours and 7 patches are stucked in Qt EWS because of this build break.

Qt EWS and Qt folks always willingly help you to fix the Qt 
build, but breaking the build for hours isn't a fairplay game ...

Please respect and don't ignore "Keeping the tree green" policy:
http://webkit.org/coding/contributing.html
"Your change must at least compile on all platforms."

[Csaba Osztrogonác]

Comment on attachment 80503 [details]
Patch

remove r+ from landed patch

[Oliver Hunt]

(In reply to comment #24)
> Reopen, because it was rolled out by http://trac.webkit.org/changeset/77113
> 
> Qt build was broken for ~6 hours and 7 patches are stucked in Qt EWS because of this build break.
> 
> Qt EWS and Qt folks always willingly help you to fix the Qt 
> build, but breaking the build for hours isn't a fairplay game ...
> 
> Please respect and don't ignore "Keeping the tree green" policy:
> http://webkit.org/coding/contributing.html
> "Your change must at least compile on all platforms."


I emailed simon for assistance, there is no clear goto person for qt js bindings.

The build failures should have been trivial for someone to fix if they had a qt build.

Also afaict the build bot output was giving invalid line numbers.

There's bindings code for qt that is not using normal webcore hashmaps so my ability to actually fix code that's using them is severely limited.

I will land this again with my approach to "working build" be compiling.  Even if the code is obviously incorrect.

Personally I would have tried being more polite to someone who actually put effort into fixing things.

[Oliver Hunt]

(In reply to comment #24)
> Reopen, because it was rolled out by http://trac.webkit.org/changeset/77113
> 
> Qt build was broken for ~6 hours and 7 patches are stucked in Qt EWS because of this build break.
> 
> Qt EWS and Qt folks always willingly help you to fix the Qt 
> build, but breaking the build for hours isn't a fairplay game ...
> 
> Please respect and don't ignore "Keeping the tree green" policy:
> http://webkit.org/coding/contributing.html
> "Your change must at least compile on all platforms."

Sorry i forgot: I also told those qt people that appeared to be on irc when i logged off about what they needed to do to fix the build.  

Clearly I'm a bastard.