Generate code for store buffer updates in open-coded object field stores.

Generate code for store buffer updates in open-coded object field stores.

Committed: https://code.google.com/p/dart/source/detail?r=8755

[srdjan, 2012-06-08 05:46:58]

DBC and what about x64?

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1392: pushal();
This is bad idea: stack may contain only objects, and registers may not have
objects. No need to preserve anything, at least not here. Check callers if value
or object needs to be preserved.

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1402: const FieldAddress& dest) {
Why do you need 'object' ?

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:1569: RawObject* dest = arguments.At(0);
You may want to do Object::CheckedHandle so that you guarantee that the object
is a an object?

[Ivan Posva, 2012-06-08 14:39:55]

https://chromiumcodereview.appspot.com/10536067/diff/2001/runtime/vm/assemble...
File runtime/vm/assembler_ia32.cc (right):

https://chromiumcodereview.appspot.com/10536067/diff/2001/runtime/vm/assemble...
runtime/vm/assembler_ia32.cc:1391: // A store buffer update is required.
I was hoping that you could do the regular store into the per-isolate store
buffer right here and only call the runtime when the store buffer was full. The
easiest way is to store, increment and check for fullness. Then you do not need
to pass any parameter to the runtime call.

https://chromiumcodereview.appspot.com/10536067/diff/2001/runtime/vm/code_gen...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/10536067/diff/2001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:1567: DEFINE_RUNTIME_ENTRY(StoreBuffer, 1) {
This then would be StoreBuffer_Process.

[siva, 2012-06-08 17:26:01]

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1383: // Check that value is a new object.  Store
buffer updates are not
Check that 'value'

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1387: // Check that object is an old object.  A
store buffer update is
Check that 'object'

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1398: }
Maybe we need two versions of this one for calls from the stub code and one when
called from the code generator.

The stub code version could just save all registers if needed as we will still
be looking for tagged values when iterating stub frames.

The code gen version could do the custom preserve state stuff along with a stack
map etc.

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:1571:
Isolate::Current()->store_buffer()->AddPointer(ptr);
isolate->store_buffer()....

isolate is a param to runtime entry calls and is already set up.

[cshapiro, 2012-06-08 23:48:04]

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1383: // Check that value is a new object.  Store
buffer updates are not
Fixed.

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1387: // Check that object is an old object.  A
store buffer update is
Fixed.

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1391: // A store buffer update is required.
Passing arguments is not a problem.  The runtime call unconditionally trashes 2
registers to setup the call and then native code will trash the rest.  Until we
can ask the code generator to spill live values around such operations, we
cannot generally solve this problem.

I think this makes a very strong case for a card marking scheme instead of the
sequential-store-buffer.  The generated code for card marking is shorter,
requires fewer registers, does not have branches, and does not require runtime
calls or deduplication.

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1392: pushal();
It seems that, thanks to the filter, we can get away with preserving the
arguments to StoreIntoObject.  This may not always be true.

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1402: const FieldAddress& dest) {
Yes, this code does not make use of object.  However, the liveness-tracing tool
needs to trace all assignments and I would like to retain access to the
destination object in this context.  If persisting this additional register of
state is burdensome to the compiler we should discuss an alternative.

In the meantime, I am working on replacing this call with a general routine for
storing a known old value.  This will check the value for old space residency in
debug mode and unconditionally proceed in release mode.

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:1569: RawObject* dest = arguments.At(0);
Unfortunately, I cannot.  dest is not an object, it is just a field address.

http://codereview.chromium.org/10536067/diff/2001/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:1571:
Isolate::Current()->store_buffer()->AddPointer(ptr);
Fixed.

[cshapiro, 2012-06-12 02:35:19]

PTAL

I believe this still runs afoul of the garbage collector, but it nevertheless
addresses the remaining review comments.

[srdjan, 2012-06-12 06:57:08]

DBC

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1411: movl(dest, value);
I do not think this works as the inlined value is not marked to be an object. I
would pass a const Object& instead of const Immediate&

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:1569: RawObject* dest = arguments.At(0);
I would use the CheckedHandle thus verifying that an object is being passed.

[Ivan Posva, 2012-06-12 13:43:58]

Let's discuss the rest in person later in the week.

-Ivan

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/assembler_ia32.h
File runtime/vm/assembler_ia32.h (right):

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/assembler_ia32.h...
runtime/vm/assembler_ia32.h:524: void StoreIntoObjectNoBarrier(Register object,
I don't think you need the object for these two stores.

[cshapiro, 2012-06-12 21:41:01]

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1382: pushl(dest);  // Push argument
Apparently, this store is not legal since it is an interior pointer and not an
object pointer.

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/assembler_ia32.c...
runtime/vm/assembler_ia32.cc:1411: movl(dest, value);
Thanks.  I have changed the code to use EmitObject.  I am reluctant to provide a
dual of "LoadObject" because any "StoreObject" must go through the write barrier
to preserve invariants needed by the garbage collector.  These seems a little
bit messy because it buries a MOV in the assembler code.  This is the assembler,
after all.  Maybe I am over-thinking this?

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/assembler_ia32.h
File runtime/vm/assembler_ia32.h (right):

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/assembler_ia32.h...
runtime/vm/assembler_ia32.h:524: void StoreIntoObjectNoBarrier(Register object,
If you are suggesting that a minimal implementation does not need the parameter,
I do understand that.  But, there is more going on here than just a field
assignment.

The object argument is used by the implementation of this method.  This is used
to validate the safety of the store in debug mode.  Also, for this method, and
the method below, I need the object for implementing tracing of assignments to
heap objects.

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/10536067/diff/5002/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:1569: RawObject* dest = arguments.At(0);
This is not an object, just a field address.  I think you made the same
observation in the previous round.  I will remove the assignment to RawObject
(casting directly to a uword) and add a comment.

[cshapiro, 2012-06-15 20:50:57]

PTAL

[srdjan, 2012-06-15 21:10:34]

http://codereview.chromium.org/10536067/diff/12001/runtime/vm/code_generator_...
File runtime/vm/code_generator_ia32.cc (right):

http://codereview.chromium.org/10536067/diff/12001/runtime/vm/code_generator_...
runtime/vm/code_generator_ia32.cc:864: const Object& raw_null =
Object::ZoneHandle(Object::null());
Object::ZoneHandle() is enough to get null handle. Rename raw_null ->
null_object, actually better:
     __ StoreIntoObjectNoBarrier(EAX, FieldAddress(..), Object::ZoneHandle())

[srdjan, 2012-06-15 21:12:08]

http://codereview.chromium.org/10536067/diff/12001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

http://codereview.chromium.org/10536067/diff/12001/runtime/vm/assembler_ia32....
runtime/vm/assembler_ia32.cc:1384: CallRuntime(kStoreBufferRuntimeEntry);
I'd rather have a CallLeafRuntime which would store/restore all caller saved
registers. This way, the compiler has to spill all caller-saved registers across
each StoreIntoObject.

[cshapiro, 2012-06-15 23:17:32]

PTAL

http://codereview.chromium.org/10536067/diff/12001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

http://codereview.chromium.org/10536067/diff/12001/runtime/vm/assembler_ia32....
runtime/vm/assembler_ia32.cc:1384: CallRuntime(kStoreBufferRuntimeEntry);
Per our off-line discussion, I have added a pushal and popal around the
CallRuntime invocation.

[srdjan, 2012-06-15 23:43:43]

lgtm