Precompilation: compact the symbol table.

Precompilation: compact the symbol table.

Also, don't leak handles out of the precompiler.

dart2js/x64:
symbols 62126 -> 38668 (-37.8%)
snapshot 22534723 -> 21740838 (-3.5%)

R=asiva@google.com

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

[rmacnak, 2016-01-27 17:41:27]

Description was changed from

==========
Precompilation: compact the symbol table.
==========

to

==========
Precompilation: compact the symbol table.

Also, don't leak handles out of the precompiler.

dart2js/x64:
symbols 62126 -> 38668 (-37.8%)
snapshot 22534723 -> 21740838 (-3.5%)
==========

[rmacnak, 2016-01-27 17:52:18]

rmacnak@google.com changed reviewers:
+ asiva@google.com

[siva, 2016-01-27 21:21:11]

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

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:373: // 3. Walk the heap and built a new table from
surviving symbols.
Walk the heap and build a new

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:400:
Array::Handle(HashTables::New<SymbolTable>(kInitialSymtabSize,
Instead of kInitialSymtabSize you could count the number of symbols seen in the
iterator and use that (+ number of Predefined symbols, see comment below).

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:411:
isolate->object_store()->set_symbol_table(table.Release());
Instead of creating a strong reference to the predefined symbols above why not
just call Symbols::AddPredefinedSymbolsToIsolate() here so that they get added
to the new symbol table.

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.h#ne...
runtime/vm/symbols.h:576: // freed symbols.
Maybe

"Returns the number of symbols that have been deleted from the symbol table as
they are not referenced anywhere."

[rmacnak, 2016-01-27 23:06:20]

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

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:373: // 3. Walk the heap and built a new table from
surviving symbols.
On 2016/01/27 21:21:10, siva wrote:
> Walk the heap and build a new 

Done.

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:400:
Array::Handle(HashTables::New<SymbolTable>(kInitialSymtabSize,
On 2016/01/27 21:21:10, siva wrote:
> Instead of kInitialSymtabSize you could count the number of symbols seen in
the
> iterator and use that (+ number of Predefined symbols, see comment below).

Done. (plus load factor)

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:411:
isolate->object_store()->set_symbol_table(table.Release());
On 2016/01/27 21:21:10, siva wrote:
> Instead of creating a strong reference to the predefined symbols above why not
> just call Symbols::AddPredefinedSymbolsToIsolate() here so that they get added
> to the new symbol table.

As discussed, would leave some read-only handles pointing to garbage.

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.h#ne...
runtime/vm/symbols.h:576: // freed symbols.
On 2016/01/27 21:21:10, siva wrote:
> Maybe
> 
> "Returns the number of symbols that have been deleted from the symbol table as
> they are not referenced anywhere."

Done.

[siva, 2016-01-28 00:04:00]

lgtm

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

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:342:
GrowableObjectArray::Handle(GrowableObjectArray::New());
Can this be moved inside the scope of
if (Object::vm_isolate_sna...... == 0) {
as
GrowableObjectArray& predefined_symbols =
 
GrowableObjectArray::Handle(GrowableObjectArray::New(Symbols::kMaxPredefinedId));

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:344: String& symbol = String::Handle();
This handle could also go inside the scope of
if (Object::vm_isolate..... == 0) {

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:382: zone_(zone) {}
Why not make the signature
SymbolCollector(Thread* thread, &symbols)
  : ObjectVisitor(thread->isolate()),
    symbols_(symbols),
    zone_(thread->zone()) {}

to be consistent with other signatures.

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:385: if (obj->IsString() && obj->IsCanonical()) {
would if (obj->IsCanonical() && obj->IsString()) be more optimal?

[rmacnak, 2016-01-28 17:53:09]

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

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:342:
GrowableObjectArray::Handle(GrowableObjectArray::New());
On 2016/01/28 00:04:00, siva wrote:
> Can this be moved inside the scope of
> if (Object::vm_isolate_sna...... == 0) {
> as
> GrowableObjectArray& predefined_symbols =
>  
>
GrowableObjectArray::Handle(GrowableObjectArray::New(Symbols::kMaxPredefinedId));

Done.

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:344: String& symbol = String::Handle();
On 2016/01/28 00:03:59, siva wrote:
> This handle could also go inside the scope of
> if (Object::vm_isolate..... == 0) {

Done.

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:382: zone_(zone) {}
On 2016/01/28 00:04:00, siva wrote:
> Why not make the signature
> SymbolCollector(Thread* thread, &symbols)
>   : ObjectVisitor(thread->isolate()),
>     symbols_(symbols),
>     zone_(thread->zone()) {}
> 
> to be consistent with other signatures.

Done.

https://codereview.chromium.org/1638143002/diff/20001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:385: if (obj->IsString() && obj->IsCanonical()) {
On 2016/01/28 00:04:00, siva wrote:
> would if (obj->IsCanonical() && obj->IsString()) be more optimal?

Probably a little bit since canonical is a bit check and string is a range
check.

[rmacnak, 2016-01-28 18:23:18]

Description was changed from

==========
Precompilation: compact the symbol table.

Also, don't leak handles out of the precompiler.

dart2js/x64:
symbols 62126 -> 38668 (-37.8%)
snapshot 22534723 -> 21740838 (-3.5%)
==========

to

==========
Precompilation: compact the symbol table.

Also, don't leak handles out of the precompiler.

dart2js/x64:
symbols 62126 -> 38668 (-37.8%)
snapshot 22534723 -> 21740838 (-3.5%)

R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/c7efe5a7563f0027d2739785b926e74737aa34b9
==========

[rmacnak, 2016-01-28 18:23:19]

Committed patchset #4 (id:60001) manually as
c7efe5a7563f0027d2739785b926e74737aa34b9 (presubmit successful).