Add per-library cache of exported names

Add per-library cache of exported names

Speeds up name resolution of code that uses extensive
“exports” clauses. The pathological example in the bug report
compiles about 50 times faster.

Introduces a trail of visited libraries to detect cycles when following the exports directives. This works better with the background compiler.

BUG=26145
Committed: https://github.com/dart-lang/sdk/commit/b4ac99a4121769b132499eef3a15b9a70f0e1fce

[hausner, 2016-04-18 21:10:04]

Description was changed from

==========
Add per-library cache of exported names

Speeds up name resolution of code that uses extensive
“exports” clauses.

BUG= 26145
==========

to

==========
Add per-library cache of exported names

Speeds up name resolution of code that uses extensive
“exports” clauses.

Introduces a trail of visited libraries to detect cycles when following the
exports directives. This works better with the background compiler.

BUG= 26145
==========

[hausner, 2016-04-18 21:11:50]

Description was changed from

==========
Add per-library cache of exported names

Speeds up name resolution of code that uses extensive
“exports” clauses.

Introduces a trail of visited libraries to detect cycles when following the
exports directives. This works better with the background compiler.

BUG= 26145
==========

to

==========
Add per-library cache of exported names

Speeds up name resolution of code that uses extensive
“exports” clauses. The pathological example in the bug report
compiles about 50 times faster.

Introduces a trail of visited libraries to detect cycles when following the
exports directives. This works better with the background compiler.

BUG= 26145
==========

[hausner, 2016-04-18 21:11:50]

hausner@google.com changed reviewers:
+ iposva@google.com, srdjan@google.com

[srdjan, 2016-04-18 22:40:49]

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

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:60: DEFINE_FLAG(bool, use_exp_cache, true, "Use library
exported name cache");
use_exported_name_cache sounds more readable than use_exp_cache.

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9627: if (!FLAG_use_lib_cache ||
Compiler::IsBackgroundCompilation()) {
Why are we starting to call AddToResolvedNamesCache from BackgroundCompiler?

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9646: // change 'resolved_names()' while running a
background compilation;
s/resolved_names/exported_names/

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9648: cache.Release();
You may want to the same as in LookupResolvedNamesCache (changed recently):

#if defined(DEBUG)
  if (Thread::Current()->IsMutatorThread()) {
    ASSERT(cache.Release().raw() == exported_names());
  } else {
    // Release must be called in debug mode.
    cache.Release();
  }
#endif

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9655: if (!FLAG_use_exp_cache ||
Compiler::IsBackgroundCompilation()) {
Why tests for background compilation when asserting it can't be?

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9667: void Library::InvalidateResolvedName(const String&
name) const {
You may want to:

Thread* thread = Thread::Current();
Zone* zone = ..
Isolate* isolate = ...

then pass zone to handles and replace call to Isolate::Current() with isolate.

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9667: void Library::InvalidateResolvedName(const String&
name) const {
ASSERT(FLAG_use_exp_cache)

[Ivan Posva, 2016-04-18 23:07:21]

Thanks,
-Ivan

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

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:60: DEFINE_FLAG(bool, use_exp_cache, true, "Use library
exported name cache");
In the future please add new flags to the flag_list.h. I am assuming this flag
is temporary while we test the impact of this feature, correct?

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9673: GrowableObjectArray& libs =
GrowableObjectArray::Handle(
Please add a comment how we can get into a state where this removal is needed
and why we need to invalidate all re-export caches.

[hausner, 2016-04-18 23:51:58]

Thank you.

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

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:60: DEFINE_FLAG(bool, use_exp_cache, true, "Use library
exported name cache");
On 2016/04/18 22:40:49, srdjan wrote:
> use_exported_name_cache sounds more readable than use_exp_cache.

More readable maybe, but I'm not sure it is more understandable. This option
should not be used by the general public. If you insist, I can make it
use_export_cache.

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:60: DEFINE_FLAG(bool, use_exp_cache, true, "Use library
exported name cache");
On 2016/04/18 23:07:21, Ivan Posva wrote:
> In the future please add new flags to the flag_list.h. I am assuming this flag
> is temporary while we test the impact of this feature, correct?

Yes, I consider the flag temporary.

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9627: if (!FLAG_use_lib_cache ||
Compiler::IsBackgroundCompilation()) {
On 2016/04/18 22:40:48, srdjan wrote:
> Why are we starting to call AddToResolvedNamesCache from BackgroundCompiler?

No change here, but I think it is possible that the BG compiler ends up calling
this function, which is a latent race condition.

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9646: // change 'resolved_names()' while running a
background compilation;
On 2016/04/18 22:40:49, srdjan wrote:
> s/resolved_names/exported_names/

Done.

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9648: cache.Release();
On 2016/04/18 22:40:49, srdjan wrote:
> You may want to the same as in LookupResolvedNamesCache (changed recently):
> 
> #if defined(DEBUG)
>   if (Thread::Current()->IsMutatorThread()) {
>     ASSERT(cache.Release().raw() == exported_names());
>   } else {
>     // Release must be called in debug mode.
>     cache.Release();
>   }
> #endif

Done.

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9655: if (!FLAG_use_exp_cache ||
Compiler::IsBackgroundCompilation()) {
On 2016/04/18 22:40:48, srdjan wrote:
> Why tests for background compilation when asserting it can't be?

Ok, assertion removed.

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9667: void Library::InvalidateResolvedName(const String&
name) const {
On 2016/04/18 22:40:49, srdjan wrote:
> ASSERT(FLAG_use_exp_cache)

the flags for resolved name cache and exported names cache are independent. We
can get here even if the export cache is not used.

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9667: void Library::InvalidateResolvedName(const String&
name) const {
On 2016/04/18 22:40:49, srdjan wrote:
> You may want to:
> 
> Thread* thread = Thread::Current();
> Zone* zone = ..
> Isolate* isolate = ...
> 
> then pass zone to handles and replace call to Isolate::Current() with isolate.

Done.

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9673: GrowableObjectArray& libs =
GrowableObjectArray::Handle(
On 2016/04/18 23:07:21, Ivan Posva wrote:
> Please add a comment how we can get into a state where this removal is needed
> and why we need to invalidate all re-export caches.

Done.

[hausner, 2016-04-19 16:10:52]

Description was changed from

==========
Add per-library cache of exported names

Speeds up name resolution of code that uses extensive
“exports” clauses. The pathological example in the bug report
compiles about 50 times faster.

Introduces a trail of visited libraries to detect cycles when following the
exports directives. This works better with the background compiler.

BUG= 26145
==========

to

==========
Add per-library cache of exported names

Speeds up name resolution of code that uses extensive
“exports” clauses. The pathological example in the bug report
compiles about 50 times faster.

Introduces a trail of visited libraries to detect cycles when following the
exports directives. This works better with the background compiler.

BUG= 26145

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

[hausner, 2016-04-19 16:10:54]

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

[hausner, 2016-04-19 16:23:27]

I received your verbal LGTM yesterday.

[srdjan, 2016-04-26 21:09:42]

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

https://codereview.chromium.org/1902563002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:60: DEFINE_FLAG(bool, use_exp_cache, true, "Use library
exported name cache");
On 2016/04/18 23:51:58, hausner wrote:
> On 2016/04/18 22:40:49, srdjan wrote:
> > use_exported_name_cache sounds more readable than use_exp_cache.
> 
> More readable maybe, but I'm not sure it is more understandable. This option
> should not be used by the general public. If you insist, I can make it
> use_export_cache.

The flag is temporary, your choice.