Remove fixed contant pool entries by caching some global constants in Thread

Remove fixed contant pool entries by caching some global constants in Thread

They can be loaded via the THR register.

In particular, do this for null, true, false, and the entry point of the UpdateStoreBufferStub.

This will help supporting precompilation and reduce memory usage from constant pools (assuming there are more constant pools than threads).

R=srdjan@google.com

Committed: https://github.com/dart-lang/sdk/commit/3b8cbf8bfa53797e100e96f70ea57677f3006b46

[Florian Schneider, 2015-07-07 12:21:41]

fschneider@google.com changed reviewers:
+ koda@google.com, srdjan@google.com

[Florian Schneider, 2015-07-07 12:21:41]

This is based on https://codereview.chromium.org/1210033007/, but only affects
x64 and arm64 where the fixed pool entries are removed for now. Other platforms
will be changed as needed in a new CL.

[koda, 2015-07-07 13:43:58]

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:212: || (object.raw() == Bool::True().raw())
It would be safer to connect this with the code that initializes the cache.
Perhaps making this a non-static method that compares to the actual fields
instead?

Or, if we expect to add more of these in the future, a visitor macro pattern
(type + field name + value expression, for each constant). That would minimize
risk of copy-paste errors, but might be overkill though.

At least, please use same expression for each value (Bool::True vs
Object::bool_true, etc.) in this file.

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:217: intptr_t Thread::OffsetFromThread(const Object&
object) {
ASSERT(CanLoadFromThread)

[srdjan, 2015-07-07 16:58:27]

LGTM

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.cc (right):

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.cc:368: bool Assembler::CanLoadFromObjectPool(const
Object& object) {
const

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/assembler_x6...
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:2775: bool Assembler::CanLoadFromObjectPool(const
Object& object) {
const

[Florian Schneider, 2015-07-08 09:28:09]

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.cc (right):

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.cc:368: bool Assembler::CanLoadFromObjectPool(const
Object& object) {
On 2015/07/07 16:58:27, srdjan wrote:
> const

Done.

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/assembler_x6...
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:2775: bool Assembler::CanLoadFromObjectPool(const
Object& object) {
On 2015/07/07 16:58:27, srdjan wrote:
> const

Done.

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:212: || (object.raw() == Bool::True().raw())
On 2015/07/07 13:43:57, koda wrote:
> It would be safer to connect this with the code that initializes the cache.
> Perhaps making this a non-static method that compares to the actual fields
> instead?
> 
> Or, if we expect to add more of these in the future, a visitor macro pattern
> (type + field name + value expression, for each constant). That would minimize
> risk of copy-paste errors, but might be overkill though.
> 
> At least, please use same expression for each value (Bool::True vs
> Object::bool_true, etc.) in this file.

Macro sounds good since there are a few of places to keep in sync.

https://codereview.chromium.org/1225933002/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:217: intptr_t Thread::OffsetFromThread(const Object&
object) {
On 2015/07/07 13:43:57, koda wrote:
> ASSERT(CanLoadFromThread)

Done.

[Florian Schneider, 2015-07-08 10:27:59]

@koda: I added macros to generate declarations/definitions and helpers for the
cached objects. Ptal.

[Florian Schneider, 2015-07-08 11:37:52]

Committed patchset #3 (id:40001) manually as
3b8cbf8bfa53797e100e96f70ea57677f3006b46 (presubmit successful).

[koda, 2015-07-08 13:13:10]

lgtm

https://codereview.chromium.org/1225933002/diff/40001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1225933002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:73: member_name = default_init_value;
Too much indentation.

https://codereview.chromium.org/1225933002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:85: member_name = init_expr;
For the objects, we should probably ASSERT they are actually in the VM isolate.

Also, I suggest putting parentheses around init_expr for safety.

[Florian Schneider, 2015-07-08 13:31:35]

https://codereview.chromium.org/1225933002/diff/40001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1225933002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:73: member_name = default_init_value;
On 2015/07/08 13:13:10, koda wrote:
> Too much indentation.

Done.

https://codereview.chromium.org/1225933002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:85: member_name = init_expr;
On 2015/07/08 13:13:10, koda wrote:
> For the objects, we should probably ASSERT they are actually in the VM
isolate.
> 
> Also, I suggest putting parentheses around init_expr for safety.

Done.

Asserting using IsVMHeapObject does not work because at Thread init-time the
Isolate::Current() is still NULL, and I get a segfault.

Is there any other way to check for VM isolate objects?


#0  0x00000000004273ca in dart::BaseIsolate::IncrementNoSafepointScopeDepth
(this=0x0) at runtime/vm/base_isolate.h:86
#1  0x000000000053e156 in dart::NoSafepointScope::NoSafepointScope
(this=0x7ffff5a63d48) at runtime/vm/heap.cc:747
#2  0x00000000006684dd in dart::ExclusivePageIterator::ExclusivePageIterator
(this=0x7ffff5a63d30, space=0xc1d7b0) at runtime/vm/pages.cc:443
#3  0x00000000006657ca in dart::PageSpace::Contains (this=0xc1d7b0,
addr=140737352585248) at runtime/vm/pages.cc:532
#4  0x000000000053c1f2 in dart::Heap::Contains (this=0xc1d260,
addr=140737352585248) at runtime/vm/heap.cc:197
#5  0x00000000006b0a40 in dart::RawObject::IsVMHeapObject (this=0x7ffff7e85021)
at runtime/vm/raw_object.cc:22
#6  0x0000000000724b21 in dart::Thread::InitVMConstants (this=0x7ffff00008c0) at
runtime/vm/thread.cc:85
#7  0x0000000000724b06 in dart::Thread::Thread (this=0x7ffff00008c0,
init_vm_constants=true) at runtime/vm/thread.cc:77
#8  0x0000000000724a8c in dart::Thread::EnsureInit () at runtime/vm/thread.cc:53
#9  0x0000000000727424 in dart::ThreadPool::Worker::Main (args=12777328) at
runtime/vm/thread_pool.cc:318
#10 0x0000000000662950 in dart::ThreadStart (data_ptr=0xc2f800) at
runtime/vm/os_thread_linux.cc:86
#11 0x00007ffff7bc4182 in start_thread (arg=0x7ffff5a64700) at
pthread_create.c:312
#12 0x00007ffff6edb47d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:111

[koda, 2015-07-08 13:45:55]

https://codereview.chromium.org/1225933002/diff/40001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1225933002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:85: member_name = init_expr;
On 2015/07/08 13:31:35, Florian Schneider wrote:
> On 2015/07/08 13:13:10, koda wrote:
> > For the objects, we should probably ASSERT they are actually in the VM
> isolate.
> > 
> > Also, I suggest putting parentheses around init_expr for safety.
> 
> Done.
> 
> Asserting using IsVMHeapObject does not work because at Thread init-time the
> Isolate::Current() is still NULL, and I get a segfault.
> 
> Is there any other way to check for VM isolate objects?
> 
> 
> #0  0x00000000004273ca in dart::BaseIsolate::IncrementNoSafepointScopeDepth
> (this=0x0) at runtime/vm/base_isolate.h:86
> #1  0x000000000053e156 in dart::NoSafepointScope::NoSafepointScope
> (this=0x7ffff5a63d48) at runtime/vm/heap.cc:747
> #2  0x00000000006684dd in dart::ExclusivePageIterator::ExclusivePageIterator
> (this=0x7ffff5a63d30, space=0xc1d7b0) at runtime/vm/pages.cc:443
> #3  0x00000000006657ca in dart::PageSpace::Contains (this=0xc1d7b0,
> addr=140737352585248) at runtime/vm/pages.cc:532
> #4  0x000000000053c1f2 in dart::Heap::Contains (this=0xc1d260,
> addr=140737352585248) at runtime/vm/heap.cc:197
> #5  0x00000000006b0a40 in dart::RawObject::IsVMHeapObject
(this=0x7ffff7e85021)
> at runtime/vm/raw_object.cc:22
> #6  0x0000000000724b21 in dart::Thread::InitVMConstants (this=0x7ffff00008c0)
at
> runtime/vm/thread.cc:85
> #7  0x0000000000724b06 in dart::Thread::Thread (this=0x7ffff00008c0,
> init_vm_constants=true) at runtime/vm/thread.cc:77
> #8  0x0000000000724a8c in dart::Thread::EnsureInit () at
runtime/vm/thread.cc:53
> #9  0x0000000000727424 in dart::ThreadPool::Worker::Main (args=12777328) at
> runtime/vm/thread_pool.cc:318
> #10 0x0000000000662950 in dart::ThreadStart (data_ptr=0xc2f800) at
> runtime/vm/os_thread_linux.cc:86
> #11 0x00007ffff7bc4182 in start_thread (arg=0x7ffff5a64700) at
> pthread_create.c:312
> #12 0x00007ffff6edb47d in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
> 
> 

Ah. This should go away soon, but for now you can just put the assertions in
"CanLoadFromThread" instead.

[koda, 2015-07-08 13:54:26]

https://codereview.chromium.org/1225933002/diff/40001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1225933002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:85: member_name = init_expr;
On 2015/07/08 13:45:55, koda wrote:
> On 2015/07/08 13:31:35, Florian Schneider wrote:
> > On 2015/07/08 13:13:10, koda wrote:
> > > For the objects, we should probably ASSERT they are actually in the VM
> > isolate.
> > > 
> > > Also, I suggest putting parentheses around init_expr for safety.
> > 
> > Done.
> > 
> > Asserting using IsVMHeapObject does not work because at Thread init-time the
> > Isolate::Current() is still NULL, and I get a segfault.
> > 
> > Is there any other way to check for VM isolate objects?
> > 
> > 
> > #0  0x00000000004273ca in dart::BaseIsolate::IncrementNoSafepointScopeDepth
> > (this=0x0) at runtime/vm/base_isolate.h:86
> > #1  0x000000000053e156 in dart::NoSafepointScope::NoSafepointScope
> > (this=0x7ffff5a63d48) at runtime/vm/heap.cc:747
> > #2  0x00000000006684dd in dart::ExclusivePageIterator::ExclusivePageIterator
> > (this=0x7ffff5a63d30, space=0xc1d7b0) at runtime/vm/pages.cc:443
> > #3  0x00000000006657ca in dart::PageSpace::Contains (this=0xc1d7b0,
> > addr=140737352585248) at runtime/vm/pages.cc:532
> > #4  0x000000000053c1f2 in dart::Heap::Contains (this=0xc1d260,
> > addr=140737352585248) at runtime/vm/heap.cc:197
> > #5  0x00000000006b0a40 in dart::RawObject::IsVMHeapObject
> (this=0x7ffff7e85021)
> > at runtime/vm/raw_object.cc:22
> > #6  0x0000000000724b21 in dart::Thread::InitVMConstants
(this=0x7ffff00008c0)
> at
> > runtime/vm/thread.cc:85
> > #7  0x0000000000724b06 in dart::Thread::Thread (this=0x7ffff00008c0,
> > init_vm_constants=true) at runtime/vm/thread.cc:77
> > #8  0x0000000000724a8c in dart::Thread::EnsureInit () at
> runtime/vm/thread.cc:53
> > #9  0x0000000000727424 in dart::ThreadPool::Worker::Main (args=12777328) at
> > runtime/vm/thread_pool.cc:318
> > #10 0x0000000000662950 in dart::ThreadStart (data_ptr=0xc2f800) at
> > runtime/vm/os_thread_linux.cc:86
> > #11 0x00007ffff7bc4182 in start_thread (arg=0x7ffff5a64700) at
> > pthread_create.c:312
> > #12 0x00007ffff6edb47d in clone () at
> > ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
> > 
> > 
> 
> Ah. This should go away soon, but for now you can just put the assertions in
> "CanLoadFromThread" instead.

... although at second thought, that might actually be too much of a slowdown
(iterating over the heap every time). At least assert that they are in old space
:)