Issue 10050022: Fix lazy initialization of static fields (issue 2472).

Issue 10050022: Fix lazy initialization of static fields (issue 2472). (Closed)

Created:
8 years, 8 months ago by regis

Modified:
8 years, 8 months ago

Reviewers:
srdjan

CC:
reviews_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Fix lazy initialization of static fields (issue 2472). Committed: https://code.google.com/p/dart/source/detail?r=6438

Patch Set 3 : #

Created: 8 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+177 lines, -62 lines)			Patch
M	runtime/vm/ast.cc	View	1	1 chunk	+22 lines, -1 line	0 comments	Download
M	runtime/vm/object.h	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
M	runtime/vm/parser.cc	View	1 2	5 chunks	+152 lines, -60 lines	0 comments	Download
M	tests/language/language.status	View	1	1 chunk	+0 lines, -1 line	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

srdjan

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/ast.cc File runtime/vm/ast.cc (right): https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/ast.cc#newcode416 runtime/vm/ast.cc:416: ASSERT(!field.IsNull()); Please add a comment why the field must ...

8 years, 8 months ago (2012-04-11 19:52:51 UTC) #2

regis

Thanks! https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/ast.cc File runtime/vm/ast.cc (right): https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/ast.cc#newcode416 runtime/vm/ast.cc:416: ASSERT(!field.IsNull()); On 2012/04/11 19:52:51, srdjan wrote: > Please ...

8 years, 8 months ago (2012-04-11 21:11:24 UTC) #3

Thanks!

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/ast.cc
File runtime/vm/ast.cc (right):

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/ast.cc#newc...
runtime/vm/ast.cc:416: ASSERT(!field.IsNull());
On 2012/04/11 19:52:51, srdjan wrote:
> Please add a comment why the field must exist.

Done. I added a comment and debugging code verifying that we are indeed dealing
with lazy initialization.

    // Access to a lazily initialized static field that has not yet been
    // initialized is compiled to a static implicit const getter.
    // A setter may not exist for such a field.

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:723: if (field.is_final()) {  // TODO(regis): Should be
field.is_const().
On 2012/04/11 19:52:51, srdjan wrote:
> You could already add is_const() tester to field, which would return true for
> final fields.

Done.

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:733: // if (field.value == transition_sentinel) {
On 2012/04/11 19:52:51, srdjan wrote:
> === (strict equal used)

Done.

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:734: //   field.value = null;
On 2012/04/11 19:52:51, srdjan wrote:
> Why set it to null? If we catch the exception, next acces will be then valid?

Yes, I spoke earlier with Gilad about it. We should only get one exception.
Added comment:
    // Set field to null prior to throwing exception, so that subsequent
    // accesses to the field do not throw again, since initializers should only
    // be executed once.

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:737: // if (field.value == sentinel) {
On 2012/04/11 19:52:51, srdjan wrote:
> ===

Done.

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:6258: AstNode* initializer =
RunStaticFieldInitializer(field);
On 2012/04/11 19:52:51, srdjan wrote:
> initializer -> call_getter ?

No, the returned ast is the initializer. If the field is already initialized,
null is returned.

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:6262: if (field.is_final()) {  // TODO(regis): Should be
field.is_const().
On 2012/04/11 19:52:51, srdjan wrote:
> Add is_const() to Field

Done.

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:6729: AstNode* Parser::RunStaticFieldInitializer(const
Field& field) {
On 2012/04/11 19:52:51, srdjan wrote:
> Add comment what you are returning.

Done.

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:6737: } else {
On 2012/04/11 19:52:51, srdjan wrote:
> Add comment like: call static getter so that it can throw the exception.

Done.

https://chromiumcodereview.appspot.com/10050022/diff/1/runtime/vm/parser.cc#n...
runtime/vm/parser.cc:6786: } else {
On 2012/04/11 19:52:51, srdjan wrote:
> If I understand correctly, we will now always call the getter for non-final
> static fields, previously we loaded directly from the field. Please run
> benchmarks before/after this change to see if there are any performance
> degradations.
> 
> As I understand it, this also introduces lazy initialization of static fields.

As we discussed, it would be comparing apples and oranges, since the previous
behavior was not correct. Let's watch Golem and see if this causes a slowdown.
In that case, it will be easy for the optimizing compiler to replace calls to
the getter with direct field accesses if the field is initialized. Note that we
still access the field directly if it has been initialized by the time we
compile the field access.

lgtm

Expand Messages | Collapse Messages