674297 - IonMonkey: Make IonCode and its targets relocatable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

As we start emitting jumps into other IonCode objects, or jumps into C/C++, we need to take care of the following concerns:
 * The destination could be moved (planning ahead for moving GC)
 * The destination needs to be kept alive (marked)
 * The source buffer *is* moved (upon final reallocation)
 * GC can happen during code generation

So this bug entails a few things:
 * Add jmp(void *), call(void *) auto-relocating API to Assembler
 * Add compressed relocation tables to final code buffers
 * Trace hook for IonCode to walk its relocation table
 * Let GC scan active MacroAssemblers

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

This patch lets the assembler jump to external code. Each external jump is added to a list of pending jumps, which is patched when we move the code from the MacroAssembler into the ExecutablePool.

The two tricky parts are:
 * IonCode targets are owned by the GC, so the GC now scans through active MacroAssemblers and looks at their pending jump targets. But the targets are to raw code, not to gcthings, so in addition to making MacroAssembler rooted, a code buffer now has a secret pointer to its IonCode squirreled right before the start.

 * Similarly, during a GC, we need to find jumps from one IonCode object to another. In JM we did this with PICs but now we'll do it with relocation tables. Relocation tables are platform specific.

   * On x86, the relocation table is a list containing each jump offset. From there we can recover the targets and convert them to IonCode * pointers.

   * On x64, a signed 32-bit jump might not be enough to cover the entire address space. In this case we redirect the jump to the bottom of the script, where an extended jump table is reserved (16 bytes per jump). Note that this is a lot of overhead that will very rarely be used - but the important thing is that it works now. Two future solutions:
    * Change ExecutableAllocator to stick in a 32-bit address range.
    * Don't use bailout tables on x64, greatly reducing external references,
      and reduce the cost per jump to 10 bytes (push + jump).

Relocation tables are compressed, each entry is variable-length encoded with a helper class called CompactBuffer (this will be used for snapshots, too). They get appended to the end of the code stream.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2, fix a bunch of bugs

Comment 3

[Brian Hackett [Laid off!]]

Should Ion code be able to survive a major GC?  With TI enabled all mjit code is now discarded on GC, as the inference constraints are also discarded and keeping track of the dependencies necessary to keep the JIT code around is not worth the time/space.  Guards are used to ensure that if the compiler or analysis is active when the GC is triggered then all code and analysis data is kept alive.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #3)
> Should Ion code be able to survive a major GC?  With TI enabled all mjit
> code is now discarded on GC, as the inference constraints are also discarded
> and keeping track of the dependencies necessary to keep the JIT code around
> is not worth the time/space.  Guards are used to ensure that if the compiler
> or analysis is active when the GC is triggered then all code and analysis
> data is kept alive.

Is a major GC a cross-compartment one? Does TI throw code away to save memory? It's probably too early to tell, but we should avoid any architectural flaws that would limit our options when IM and GC interact. Examples: being able to GC from within JIT code, being able to compact JIT code (possibly while it's running?), being able to move objects while in JIT, etc. I would include "throw away live JIT code" in that list, so whenever the time comes we can just tune it.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 548727 [details] [diff] [review]
v2, fix a bunch of bugs

Bill, could you look at the gc-related changes? They're in: IonMacroAssembler.h, Assembler-x86.cpp, Assembler-x64.cpp, jsgc.cpp, jsgcmark.cpp

Comment 6

[Andrew Drake [:adrake]]

Comment on attachment 548727 [details] [diff] [review]
v2, fix a bunch of bugs

Review of attachment 548727 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/CompactBuffer.h
@@ +52,5 @@
> +    uint8 *buffer_;
> +    uint8 *end_;
> +
> +    template <typename T> T readVariableLength() {
> +        T val = 0;

This function assumes sizeof(T) == 4 everywhere. Could we get a static assert here for that?

::: js/src/ion/Ion.cpp
@@ +187,5 @@
> +IonCode::copyFrom(MacroAssembler &masm)
> +{
> +    // Store the IonCode pointer right before the code buffer, so we can
> +    // recover the gcthing from relocation tables.
> +    *(IonCode **)(code_- sizeof(IonCode *)) = this;

Minor Nit: code_ -

::: js/src/ion/x64/Assembler-x64.h
@@ +351,5 @@
> +    void jmp(void *target, Relocation::Kind reloc) {
> +        JmpSrc src = masm.jmp();
> +        addPendingJump(src, target, reloc);
> +    }
> +    void j(Condition cond, void *target, Relocation::Kind reloc) {

Can this just be called jmp too?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 548727 [details] [diff] [review]
v2, fix a bunch of bugs

Review of attachment 548727 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Would it be possible to move Assembler::TraceRelocations, Assembler::trace, RelativePatch, and jumps_ into a common location? I might be missing a difference, but the definitions in -x86 and -x64 look identical to me.

::: js/src/ion/Ion.cpp
@@ +200,5 @@
> +
> +void
> +IonCode::trace(JSTracer *trc)
> +{
> +    if (relocTableSize_) {

It doesn't look like relocTableSize_ is initialized to zero. Do you need this?

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #6)
> Can this just be called jmp too?

I'd prefer to call them by their names in the manual, maybe we should have jz/je/jle etc, or just jCC().(In reply to comment #7)

> Comment on attachment 548727 [details] [diff] [review] [review]
> Looks good. Would it be possible to move Assembler::TraceRelocations,
> Assembler::trace, RelativePatch, and jumps_ into a common location? I might
> be missing a difference, but the definitions in -x86 and -x64 look identical
> to me.
> 
> ::: js/src/ion/Ion.cpp
> @@ +200,5 @@
> > +
> > +void
> > +IonCode::trace(JSTracer *trc)
> > +{
> > +    if (relocTableSize_) {
> 
> It doesn't look like relocTableSize_ is initialized to zero. Do you need
> this?

Thanks, I'll fix both of these.

Comment 9

[Andrew Drake [:adrake]]

Comment on attachment 548727 [details] [diff] [review]
v2, fix a bunch of bugs

Review of attachment 548727 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/projects/ionmonkey/rev/af3ff9bdcc7e
http://hg.mozilla.org/projects/ionmonkey/rev/a7e1def9ff0f