Jon Coppeard provided some useful feedback on the SpiderMonkey upgrade
PR that probably shouldn't just languish in my inbox.
-------- Forwarded Message --------
Subject: Re: Reviewing the SpiderMonkey upgrade for Servo
Date: Fri, 10 Oct 2014 04:45:34 -0700 (PDT)
From: Jon Coppeard <jcoppe...@mozilla.com>
To: Josh Matthews <jmatth...@mozilla.com>
CC: Terrence Cole <tc...@mozilla.com>
I've read the guide and spent some time looking at your branch, in particular
components/script/dom/bindings/js.rs and trace.rs. My knowledge of rust is not
that great so I've probably misunderstood a few things -- apologies in advance
if my comments are off base!
Firstly, some things I really like:
- use of single GC will allow all unreachable memory to be cleaned up with one
GC vs the GC/CC/GC cycle we have in Firefox
- use of Encodable trait to auto-generate trace function is neat and saves all
that CC macro faff
- use of lifetimes parameters to ensure that a JSRef<T> doesn't outlive its
referent
- reference to "the whims of the SpiderMonkey garbage collector" :-)
Potential issues I see with the design:
- there are Root, Handle and MutableHandle types that work on SM GC things,
but there's no equivalent of the rooting analysis (that I could see) that
prevents you from keeping unrooted GC thing pointers live over an operation
that could GC.
- even though raw DOM nodes aren't much use on their own, there's nothing to
people using them and passing them around and then they won't be traced.
Probably not a big deal.
Potential issues with the implementation:
- I think the idea is for the reflector JS Object to have a finalize hook that
destroys the rust DOM node. I couldn't find where this happens though --
finalizeHook in CodegenRust.py doesn't seem to actually do anything -- but
maybe this is not implemented yet.
- The structures PersistentRootedObjectElement and Root try to match their C++
equivalents, and as far as I can see are actually added to SM's internal data
structures. This is pretty fragile obviously. We could add a callback to
allow an embedding to trace additional roots on minor GC so that Servo could
maintain its own lists of these things.
- As noted in the FIXME trace_object() doesn't update the pointer if the
object moved, so this won't work with compacting GC (or indeed with
generational if you were to trace roots with a callback). The trace functions
always take a double indirect pointer and it would be great if the Servo design
would allow this too.
- similarly objectPostBarrier / objectRelocate require a double indirect
pointer but it doesn't look like that's what's being passed
- LiveDOMReferences has a hashtable keyed on JSObject pointers which won't
work with compacting unless swept and rekeyed. In general all hashtables
containing GC pointers as either keys or values require special attention. If
there are others then you might want to get them looked over as well.
Other:
- servo/components/script/dom/bindings/DESIGN.md has grammatical errors
- some documentation mentions the conservative stack scanner. I don't know if
this refers to something in rust, but the SM one has been removed.
- as discussed on IRC there are no postbarriers for JS<T> but that's ok
because these object must always be tenured as they have a finalizer
Cheers,
Jon
----- Original Message -----
From: "Josh Matthews" <jmatth...@mozilla.com>
To: "Jon Coppeard" <jcoppe...@mozilla.com>
Cc: "Terrence Cole" <tc...@mozilla.com>
Sent: Thursday, October 2, 2014 3:25:53 PM
Subject: Re: Reviewing the SpiderMonkey upgrade for Servo
To be clear, I assume you mean direct pointers to the reflectors, not
the DOM data (which is allocated in the Rust heap and therefore doesn't
move around). We don't actually go through the reflectors in most cases;
instead we store pointers to Rust objects (inside smart pointers that
the GC traces), and you can obtain the object's reflector from the
Reflector field that each type contains. Reflectors hold raw JSObject
pointers, so that's probably a hazard that needs to be addressed
(probably not before enabling the moving GC, though, right?).
Cheers,
Josh
On 2014-10-02 10:17 AM, Jon Coppeard wrote:
Hi there,
I'll try and get time to look at this in the next couple of days. Bear in mind
that I don't know rust so don't expect detailed comments on your implementation
;) I should be able to comment on the GC related design though.
First question: it seem that for SM to function a GC for rust native DOM
objects, all pointers between rust native objects must go via the JS reflector
object. Did I understand that correctly? If so how do you enforce that there
are no direct pointers?
Jon
----- Original Message -----
From: "Josh Matthews" <jmatth...@mozilla.com>
To: jcoppe...@mozilla.com, tc...@mozilla.com
Sent: Tuesday, September 30, 2014 2:44:13 AM
Subject: Reviewing the SpiderMonkey upgrade for Servo
Hello! I'm trying to stop rebasing the SM upgrade for Servo endlessly,
so I'm looking for someone who knows SpiderMonkey well who can focus on
parts like components/script/dom/bindings/*.rs in
https://github.com/servo/servo/pull/3527 and look over
https://github.com/servo/rust-mozjs/pull/106 to make sure that nothing
sticks out. Are either of you up to the challenge? I'm happy to provide
whatever background information you feel you require.
Cheers,
Josh
_______________________________________________
dev-servo mailing list
dev-servo@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-servo
