Issue 17028011: Implement a list tracer phase, that tries to find element types in individual lists.

Issue 17028011: Implement a list tracer phase, that tries to find element types in individual lists. (Closed)

Created:
7 years, 6 months ago by ngeoffray

Modified:
7 years, 6 months ago

Reviewers:
kasperl

CC:
reviews_dartlang.org, ahe, karlklose, Johnni Winther, polux, sra1

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

Visibility:
Public.

More Reviews

Description

Implement a list tracer phase, that tries to find element types in individual lists. R=kasperl@google.com Committed: https://code.google.com/p/dart/source/detail?r=24093

Patch Set 3 : #

Created: 7 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1237 lines, -142 lines)			Patch
M	sdk/lib/_internal/compiler/implementation/compiler.dart	View	1	2 chunks	+2 lines, -0 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/dart2jslib.dart	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/ssa/builder.dart	View		2 chunks	+10 lines, -3 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/ssa/invoke_dynamic_specializers.dart	View		1 chunk	+11 lines, -11 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/ssa/tracer.dart	View		2 chunks	+9 lines, -7 lines	0 comments	Download
A	sdk/lib/_internal/compiler/implementation/types/container_tracer.dart	View	1 2	1 chunk	+666 lines, -0 lines	0 comments	Download
A	sdk/lib/_internal/compiler/implementation/types/container_type_mask.dart	View	1	1 chunk	+138 lines, -0 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/types/flat_type_mask.dart	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart	View	1	15 chunks	+24 lines, -17 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart	View		13 chunks	+73 lines, -86 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/types/type_mask.dart	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/types/types.dart	View	1	6 chunks	+68 lines, -16 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/types/union_type_mask.dart	View		4 chunks	+10 lines, -1 line	0 comments	Download
M	tests/compiler/dart2js/concrete_type_inference_test.dart	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
A	tests/compiler/dart2js/list_tracer_test.dart	View	1	1 chunk	+220 lines, -0 lines	0 comments	Download
M	tests/compiler/dart2js/mock_compiler.dart	View	1	2 chunks	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

ngeoffray

Unit tests to come, but the code should be in a reasonable form for review.

7 years, 6 months ago (2013-06-14 11:37:09 UTC) #1

kasperl

Looks pretty good -- looking forward to seeing the tests. https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler/implementation/types/container_type_mask.dart File sdk/lib/_internal/compiler/implementation/types/container_type_mask.dart (right): https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler/implementation/types/container_type_mask.dart#newcode9 ...

7 years, 6 months ago (2013-06-17 07:43:30 UTC) #2

ngeoffray

Thanks Kasper. I addressed your comments and added a unit test. PTAL. https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler/implementation/types/container_type_mask.dart File sdk/lib/_internal/compiler/implementation/types/container_type_mask.dart ...

7 years, 6 months ago (2013-06-17 08:59:36 UTC) #3

Thanks Kasper. I addressed your comments and added a unit test. PTAL.

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/types/container_type_mask.dart
(right):

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/container_type_mask.dart:9: ///
have the same.
On 2013/06/17 07:43:30, kasperl wrote:
> have the same what?

ElementTypeHolder. I rephrased the comment.

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/container_type_mask.dart:31: //
[ContainerTypeMask] of the same node.
On 2013/06/17 07:43:30, kasperl wrote:
> of -> for

Done.

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/container_type_mask.dart:43: :
this.holder = holder == null ? new ElementTypeHolder() : holder;
On 2013/06/17 07:43:30, kasperl wrote:
> Nit: ( ) around the == comparison.

Done.

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart
(right):

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/inferrer_visitor.dart:226:
locals = handler == null ? new LocalsHandler(inferrer, compiler) : handler;
On 2013/06/17 07:43:30, kasperl wrote:
> Nit: ( ) around the == comparison.

Done.

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/types/list_tracer.dart (right):

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/list_tracer.dart:5: library
list_tracer;
On 2013/06/17 07:43:30, kasperl wrote:
> Suggestion: Rename to container tracer? It seems plausible we could extend
this
> to sets and maps too.

Done.

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/list_tracer.dart:17: * change
the element type of the list.
On 2013/06/17 07:43:30, kasperl wrote:
> Maybe add 'or let the list escape to code that might change the element type'
to
> the comment?
> 
> Is there any way this could (long term) be computed instead of hard coded?
> Maintaining a list like this is always difficult.

Yeah, I agree it's brittle. We could theorically let the analysis determine it,
but do we want to analyze those methods every time we run the compiler? I don't
have nice ideas right now on how to compute it.

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/list_tracer.dart:80: bool
_VERBOSE = false;
On 2013/06/17 07:43:30, kasperl wrote:
> Do you want to keep this logging around?

Yes, it's very useful to figure out why a certain list did not get a type (the
'bailout' method).

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/list_tracer.dart:128: static
const int MAX_ANALYSIS_COUNT = 10;
On 2013/06/17 07:43:30, kasperl wrote:
> Clearly this needs to be a proper number :-)

Err, my bad. Pushed it to 11.

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/list_tracer.dart:131: bool
bailoutAnalysis = false;
On 2013/06/17 07:43:30, kasperl wrote:
> I think you should change the name of this flag. It sounds like you're
analyzing
> bailout methods or something. Maybe negate it and have it be
continueAnalysing?

Done.

https://codereview.chromium.org/17028011/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/list_tracer.dart:201: void
addSettersToAnalysis(Selector selector) {
On 2013/06/17 07:43:30, kasperl wrote:
> You could try keeping track of the selectors you've already seen (in a set)
and
> avoid repeated filtering. It might pay off for big methods (lots of setter
> calls).

Done.

kasperl

LGTM. https://codereview.chromium.org/17028011/diff/9001/sdk/lib/_internal/compiler/implementation/types/container_tracer.dart File sdk/lib/_internal/compiler/implementation/types/container_tracer.dart (right): https://codereview.chromium.org/17028011/diff/9001/sdk/lib/_internal/compiler/implementation/types/container_tracer.dart#newcode98 sdk/lib/_internal/compiler/implementation/types/container_tracer.dart:98: mask.elementType = Long line. https://codereview.chromium.org/17028011/diff/9001/sdk/lib/_internal/compiler/implementation/types/container_tracer.dart#newcode135 sdk/lib/_internal/compiler/implementation/types/container_tracer.dart:135: static const ...

7 years, 6 months ago (2013-06-17 12:01:15 UTC) #4

ngeoffray

Thanks Kasper! https://codereview.chromium.org/17028011/diff/9001/sdk/lib/_internal/compiler/implementation/types/container_tracer.dart File sdk/lib/_internal/compiler/implementation/types/container_tracer.dart (right): https://codereview.chromium.org/17028011/diff/9001/sdk/lib/_internal/compiler/implementation/types/container_tracer.dart#newcode98 sdk/lib/_internal/compiler/implementation/types/container_tracer.dart:98: mask.elementType = On 2013/06/17 12:01:15, kasperl wrote: ...

7 years, 6 months ago (2013-06-17 12:03:10 UTC) #5

ngeoffray

7 years, 6 months ago (2013-06-17 12:35:06 UTC) #6

Message was sent while issue was closed.

Committed patchset #3 manually as r24093 (presubmit successful).

Expand Messages | Collapse Messages