Add exception handler cache to isolate

Add exception handler cache to isolate

This speeds up frequently throwing exception
at the same place(s) in the code.

R=fschneider@google.com, vegorov@google.com

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

[Dmitry Olshansky, 2017-02-08 12:16:07]

dmitryolsh@google.com changed reviewers:
+ fschneider@google.com, vegorov@google.com

[Florian Schneider, 2017-02-08 21:34:05]

Thanks for factoring out into this CL!

Just a few comments:

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

https://codereview.chromium.org/2683633005/diff/20001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:20: Duplicates are no allowed - check with Lookup
before insertion.
s/no/not/

https://codereview.chromium.org/2683633005/diff/20001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:52: for (intptr_t j = length_; j-- > i;) {
Can you rewrite this without the update in the test expression for better
readability?

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

https://codereview.chromium.org/2683633005/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:406: thread->isolate()->handler_info_cache.Clear();
Can we only invalidate if there was a code GC, and not at every old-space GC?

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

https://codereview.chromium.org/2683633005/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:846: HandlerInfoCache handler_info_cache;
Maybe make this private and add a public getter method to be consistent with
other members.

[Dmitry Olshansky, 2017-02-09 17:54:34]

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

https://codereview.chromium.org/2683633005/diff/20001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:20: Duplicates are no allowed - check with Lookup
before insertion.
On 2017/02/08 21:34:05, Florian Schneider wrote:
> s/no/not/

Done.

https://codereview.chromium.org/2683633005/diff/20001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:52: for (intptr_t j = length_; j-- > i;) {
On 2017/02/08 21:34:05, Florian Schneider wrote:
> Can you rewrite this without the update in the test expression for better
> readability?

Done.

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

https://codereview.chromium.org/2683633005/diff/20001/runtime/vm/heap.cc#newc...
runtime/vm/heap.cc:406: thread->isolate()->handler_info_cache.Clear();
On 2017/02/08 21:34:05, Florian Schneider wrote:
> Can we only invalidate if there was a code GC, and not at every old-space GC?

Asked Slava and basically we can't know if there was Code GC or not.

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

https://codereview.chromium.org/2683633005/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:846: HandlerInfoCache handler_info_cache;
On 2017/02/08 21:34:05, Florian Schneider wrote:
> Maybe make this private and add a public getter method to be consistent with
> other members.

Done.

[Florian Schneider, 2017-02-09 19:25:56]

LGTM.

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache.h
File runtime/vm/fixed_cache.h (right):

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:26: class FixedCache {
Please add a unit test fixed_cache_test.cc for the different cases.
(empty cache, partially full cache, full cache, insertion at start, end, middle)

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:848: 
Remove extra \n

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

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/stack_frame....
runtime/vm/stack_frame.cc:261: HandlerInfo* p = cache->Lookup(pc());
Rename p to info for readability.

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/stack_frame....
runtime/vm/stack_frame.cc:262: if (p) {
Style: use (p != NULL)

[Vyacheslav Egorov (Google), 2017-02-10 12:45:36]

lgtm

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache.h
File runtime/vm/fixed_cache.h (right):

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:13: /*
Style:

I think this should better be 

//
//
//
//

style comment.

We don't use multiline /* */ comments and when we use them we do it like this:

/*
 *
 */

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:35: explicit FixedCache(Deleter deleter = NULL) :
deleter_(deleter), length_(0) {}
maybe kSize better be called kCapacity and length_ -> size_?

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:52: for (intptr_t j = length_ - 1; j >= i; j--) {
Can this be just a memmove(...)?

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:70: ~FixedCache() { Clear(); }
Put destructor next to contructor.

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:87: Entry pairs_[kSize];  // Sorted array of pairs.
empty line before fields

[Dmitry Olshansky, 2017-02-10 16:32:15]

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache.h
File runtime/vm/fixed_cache.h (right):

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:13: /*
On 2017/02/10 12:45:35, Vyacheslav Egorov (Google) wrote:
> Style:
> 
> I think this should better be 
> 
> //
> //
> //
> //
> 
> style comment.
> 
> We don't use multiline /* */ comments and when we use them we do it like this:
> 
> /*
>  *
>  */

Done.

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:26: class FixedCache {
On 2017/02/09 19:25:55, Florian Schneider wrote:
> Please add a unit test fixed_cache_test.cc for the different cases.
> (empty cache, partially full cache, full cache, insertion at start, end,
middle)

Done.

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:35: explicit FixedCache(Deleter deleter = NULL) :
deleter_(deleter), length_(0) {}
On 2017/02/10 12:45:35, Vyacheslav Egorov (Google) wrote:
> maybe kSize better be called kCapacity and length_ -> size_?

Done.

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:52: for (intptr_t j = length_ - 1; j >= i; j--) {
On 2017/02/10 12:45:35, Vyacheslav Egorov (Google) wrote:
> Can this be just a memmove(...)?

Actually yes, as long as Key is POD. But let's keep it as it.

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:70: ~FixedCache() { Clear(); }
On 2017/02/10 12:45:36, Vyacheslav Egorov (Google) wrote:
> Put destructor next to contructor.

Done.

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/fixed_cache....
runtime/vm/fixed_cache.h:87: Entry pairs_[kSize];  // Sorted array of pairs.
On 2017/02/10 12:45:35, Vyacheslav Egorov (Google) wrote:
> empty line before fields

Done.

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:848: 
On 2017/02/09 19:25:55, Florian Schneider wrote:
> Remove extra \n

Done.

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

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/stack_frame....
runtime/vm/stack_frame.cc:261: HandlerInfo* p = cache->Lookup(pc());
On 2017/02/09 19:25:56, Florian Schneider wrote:
> Rename p to info for readability.

Done.

https://codereview.chromium.org/2683633005/diff/40001/runtime/vm/stack_frame....
runtime/vm/stack_frame.cc:262: if (p) {
On 2017/02/09 19:25:56, Florian Schneider wrote:
> Style: use (p != NULL)

Done.

[hausner, 2017-02-10 21:57:11]

hausner@google.com changed reviewers:
+ hausner@google.com

[hausner, 2017-02-10 21:57:12]

DBC

https://codereview.chromium.org/2683633005/diff/80001/runtime/vm/exceptions.h
File runtime/vm/exceptions.h (right):

https://codereview.chromium.org/2683633005/diff/80001/runtime/vm/exceptions.h...
runtime/vm/exceptions.h:95: struct HandlerInfo {
DBC: Now that this struct is not declared inside RawExceptionHandlers class, it
might make sense to give it a more descriptive name, e.g. ExceptionHandlerInfo.

[Dmitry Olshansky, 2017-02-13 10:40:00]

https://codereview.chromium.org/2683633005/diff/80001/runtime/vm/exceptions.h
File runtime/vm/exceptions.h (right):

https://codereview.chromium.org/2683633005/diff/80001/runtime/vm/exceptions.h...
runtime/vm/exceptions.h:95: struct HandlerInfo {
On 2017/02/10 21:57:12, hausner wrote:
> DBC: Now that this struct is not declared inside RawExceptionHandlers class,
it
> might make sense to give it a more descriptive name, e.g.
ExceptionHandlerInfo.

Done.

[Dmitry Olshansky, 2017-02-13 10:41:18]

Description was changed from

==========
Add exception handler cache to isolate

This speeds up frequently throwing exception
at the same place(s) in the code.
==========

to

==========
Add exception handler cache to isolate

This speeds up frequently throwing exception
at the same place(s) in the code.

R=fschneider@google.com, vegorov@google.com

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

[Dmitry Olshansky, 2017-02-13 10:41:19]

Committed patchset #6 (id:100001) manually as
a32a3a6d2f06a55041898c1a2be78184302a80d2 (presubmit successful).