Fixes slow object pool search on x64.

Fixes slow object pool search on x64.

Issue 13144.

R=iposva@google.com

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

[Florian Schneider, 2013-09-09 19:03:11]

https://codereview.chromium.org/23723008/diff/1/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/23723008/diff/1/runtime/vm/assembler_x64.h#ne...
runtime/vm/assembler_x64.h:844: typedef RawObject* Key;
The hash map contains raw pointers, but won't be visited by GC.

[zra, 2013-09-09 19:38:44]

On 2013/09/09 19:03:11, Florian Schneider wrote:
> https://codereview.chromium.org/23723008/diff/1/runtime/vm/assembler_x64.h
> File runtime/vm/assembler_x64.h (right):
> 
>
https://codereview.chromium.org/23723008/diff/1/runtime/vm/assembler_x64.h#ne...
> runtime/vm/assembler_x64.h:844: typedef RawObject* Key;
> The hash map contains raw pointers, but won't be visited by GC.

Apologies for not understanding our GC setup quite yet, but what is the
difference between storing the raw pointers in the object pool, and assembling
the immediate values into the code, as it was before? If it does make a
difference, would it be sufficient to add code to the stack frame visitor to
visit the object pool as well? Or would I have to be able to visit both the
object pool and the hash map in the Assembler object even if the hash map is
only around while code is being assembled, and goes away when it is finalized?

Thanks!
Zach

[Ivan Posva, 2013-09-09 20:03:33]

On 2013/09/09 19:38:44, zra wrote:
> On 2013/09/09 19:03:11, Florian Schneider wrote:
> > https://codereview.chromium.org/23723008/diff/1/runtime/vm/assembler_x64.h
> > File runtime/vm/assembler_x64.h (right):
> > 
> >
>
https://codereview.chromium.org/23723008/diff/1/runtime/vm/assembler_x64.h#ne...
> > runtime/vm/assembler_x64.h:844: typedef RawObject* Key;
> > The hash map contains raw pointers, but won't be visited by GC.
> 
> Apologies for not understanding our GC setup quite yet, but what is the
> difference between storing the raw pointers in the object pool, and assembling
> the immediate values into the code, as it was before? If it does make a
> difference, would it be sufficient to add code to the stack frame visitor to
> visit the object pool as well? Or would I have to be able to visit both the
> object pool and the hash map in the Assembler object even if the hash map is
> only around while code is being assembled, and goes away when it is finalized?
> 
> Thanks!
> Zach

Zach,

Let's discuss GC interactions with the hash map offline.

-Ivan

[zra, 2013-09-13 16:49:34]

PTAL

Thanks,
Zach

https://codereview.chromium.org/23723008/diff/1/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/23723008/diff/1/runtime/vm/assembler_x64.h#ne...
runtime/vm/assembler_x64.h:844: typedef RawObject* Key;
On 2013/09/09 19:03:11, Florian Schneider wrote:
> The hash map contains raw pointers, but won't be visited by GC.

After discussion with Ivan and fixing up the assertions around the object pool
in https://codereview.chromium.org/23454032/, we concluded that this is safe
because the raw pointers are all for Old objects, and because they are visited
when GC visits the object_pool_ Array. I have added a TODO and WARNING here to
explain.

[Ivan Posva, 2013-09-13 17:25:09]

LGTM with comments.

-Ivan

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2178: if (obj.raw() == Object::null()) {
What if you get a null and it should be patchable?

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:851: // also need to visit the keys here in the
HashMap.
+fschneider: After discussing this more with Zach we came to the conclusion that
we probably should use WeakTable for this, but that will need a bit more surgery
with respect to registering and unregistering these new tables with the GC. So
we are leaving this as is for now.

+zra: Please file a cleanup bug so that we do not lose track of this.

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:852: typedef RawObject* Key;
Please add a separate comment about why these typedefs are needed. The way it is
formatted now, it looks like the typedefs are related to the GC comments above.

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:863: return
String::Hash(reinterpret_cast<uint8_t*>(&key), sizeof(&key));
Please do not use a pointer as a string.

https://codereview.chromium.org/23723008/diff/22001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/23723008/diff/22001/runtime/vm/object.h#newco...
runtime/vm/object.h:361: static const Array& empty_array() {
If you want to separate the null_xyz from the rest, then please add an empty
line before empty_array as well.

[zra, 2013-09-13 17:46:18]

Thanks!

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.cc:2178: if (obj.raw() == Object::null()) {
On 2013/09/13 17:25:09, Ivan Posva wrote:
> What if you get a null and it should be patchable?

Moved the special case for Object::null() inside of the if statement below,
which ensures that the object is not patchable.

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:851: // also need to visit the keys here in the
HashMap.
On 2013/09/13 17:25:09, Ivan Posva wrote:
> +fschneider: After discussing this more with Zach we came to the conclusion
that
> we probably should use WeakTable for this, but that will need a bit more
surgery
> with respect to registering and unregistering these new tables with the GC. So
> we are leaving this as is for now.
> 
> +zra: Please file a cleanup bug so that we do not lose track of this.

Done. Issue 13305.

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:852: typedef RawObject* Key;
On 2013/09/13 17:25:09, Ivan Posva wrote:
> Please add a separate comment about why these typedefs are needed. The way it
is
> formatted now, it looks like the typedefs are related to the GC comments
above.

Done.

https://codereview.chromium.org/23723008/diff/19001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:863: return
String::Hash(reinterpret_cast<uint8_t*>(&key), sizeof(&key));
On 2013/09/13 17:25:09, Ivan Posva wrote:
> Please do not use a pointer as a string.

Done.

https://codereview.chromium.org/23723008/diff/22001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/23723008/diff/22001/runtime/vm/object.h#newco...
runtime/vm/object.h:361: static const Array& empty_array() {
On 2013/09/13 17:25:10, Ivan Posva wrote:
> If you want to separate the null_xyz from the rest, then please add an empty
> line before empty_array as well.

Done.

[zra, 2013-09-13 18:10:10]

Committed patchset #6 manually as r27494 (presubmit successful).

[Florian Schneider, 2013-09-16 09:09:48]

https://codereview.chromium.org/23723008/diff/19002/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/23723008/diff/19002/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:856: typedef RawObject* Key;
Is there a performance reason for using raw pointers here? If not, 
how about using const Object* instead of RawObject* as key? E.g. once there is a
compacting GC, the old-object argument does not hold anymore. The fewer places
that rely on old objects not moving, the better.

https://codereview.chromium.org/23723008/diff/19002/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:867: return reinterpret_cast<intptr_t>(key) >>
kObjectAlignmentLog2;
Again, using the raw pointer as hash it not good if objects move.

How about something like String::Handle(String::New(key->ToCString())).Hash()?

https://codereview.chromium.org/23723008/diff/19002/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:871: return kv.key_ == key;
If you use handles as keys, this becomes

kv.key_->raw() == key->raw()

[Ivan Posva, 2013-09-16 15:02:20]

-Ivan

https://codereview.chromium.org/23723008/diff/19002/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/23723008/diff/19002/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:856: typedef RawObject* Key;
On 2013/09/16 09:09:48, Florian Schneider wrote:
> Is there a performance reason for using raw pointers here? If not, 
> how about using const Object* instead of RawObject* as key? E.g. once there is
a
> compacting GC, the old-object argument does not hold anymore. The fewer places
> that rely on old objects not moving, the better.

You cannot hash on handles when you are trying to get an identity map of
objects. You get many different handles for one object. See the TODO and issue
above...

[Florian Schneider, 2013-09-16 15:13:36]

On 2013/09/16 15:02:20, Ivan Posva wrote:
> -Ivan
> 
> https://codereview.chromium.org/23723008/diff/19002/runtime/vm/assembler_x64.h
> File runtime/vm/assembler_x64.h (right):
> 
>
https://codereview.chromium.org/23723008/diff/19002/runtime/vm/assembler_x64....
> runtime/vm/assembler_x64.h:856: typedef RawObject* Key;
> On 2013/09/16 09:09:48, Florian Schneider wrote:
> > Is there a performance reason for using raw pointers here? If not, 
> > how about using const Object* instead of RawObject* as key? E.g. once there
is
> a
> > compacting GC, the old-object argument does not hold anymore. The fewer
places
> > that rely on old objects not moving, the better.
> 
> You cannot hash on handles when you are trying to get an identity map of
> objects. You get many different handles for one object. See the TODO and issue
> above...

Just use String::Handle(String::New(key->ToCString())).Hash()