660013 - GCMember-annotated members should not need GC_POINTER, it should be inferred by exactgc.as

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P2)

Description

[Lars T Hansen]

In principle it should be possible to see this:

  GC_DATA_BEGIN(...)

  GCMember<C> p;

  GC_DATA_END(...)

and infer that p is a GC_POINTER, without it being annotated.

The main problem I can think of is that there could be nested classes inside the GC_DATA_BEGIN / GC_DATA_END section.  Heuristically we can recognize those by looking for "class", but we'd be getting into tricky territory - there's space for error.  Still, since we'd be generating a tracer which is then compiled by the C++ compiler there's a good chance that it won't compile if the script gets it wrong.

Comment 1

[Tommy Reilly]

I didn't think GC_DATA_BEGIN was allowed to nest.  GC_CPP_EXACT can nest obviously but there's no need to allow GC_DATA_BEGIN to if we are in fact allowing it.

Comment 2

[Lars T Hansen]

The reason GC_DATA_BEGIN / GC_DATA_END can nest is to allow large messy classes to just slap a GC_DATA_BEGIN on at the beginning and a GC_DATA_END on at the end and still have internal, exactly traced classes.

Or to put it another way, there are no real restrictions on what can be inside the GC_DATA_BEGIN / GC_DATA_END section.  In particular, I would normally have all data members inside that section, traced or not.

Comment 3

[Ruchi Lohani]

Attachment: Initial patch

Removed the _IF/_IFDEF/_IFNDEF tags from the field regex. No longer need to put GC_POINTER annotation for GCMember fields. Still require GC_POINTER for DRC/DWB/DWBRC/uintptr_t etc. GC_POINTERS still exists. Existing GC_POINTER annotations dont affect. 

Note to self: docs need to be updated accordingly. 
WE#2891528 should be fixed before this gets integrated in player.

Comment 4

[Lars T Hansen]

Comment on attachment 538364 [details] [diff] [review]
Initial patch

Minor remarks:

- the _IFDEF/_IFNDEF/_IF change is desirable now but should ideally be a
  separate patch (for a separate bug), as it's not tied into this fix
  (general rule we like to stick to, it makes it *much* easier to understand
  what a patch is about)

- On line ca 945 do not test for gcmemberIndex < 0, test for == -1 because
  that's the specific value that would be there, and it's the value we're
  testing against elsewhere.

- variable new_result is not declared.  We should probably fix the code so
  that it can be compiled with -strict -AS3; as of now that's not possible
  because of an ASC bug that does not allow const fields to be initialized
  by the constructors.  The const fields could just be turned into var fields.

- There needs to be more resilience here, gcmemberFieldTag
  will miss GCMember lines that aren't formatted correctly (eg split
  across two lines), that's a step backward from what we had.  If there's a
  GCMember on a non-comment line we must require that the line is correctly
  formatted so that we can parse it.

- What happens in the case of eg "GCMember<Blah> x, y;", which is legal?  Now
  that I write that I realize that the old code does not handle that, and
  most of us do not write code like that, but it'll be easier to write it now
  that annotations are not required.

SR- but clearly going in the right direction.

Comment 5

[Ruchi Lohani]

(In reply to comment #4)
> Comment on attachment 538364 [details] [diff] [review] [review]
> Initial patch
> 
> Minor remarks:
> 
> - the _IFDEF/_IFNDEF/_IF change is desirable now but should ideally be a
>   separate patch (for a separate bug), as it's not tied into this fix
>   (general rule we like to stick to, it makes it *much* easier to understand
>   what a patch is about)

May be. But the current changes are based on an assumption that I have removed all the _IF/_IFDEF from the code. I did not want to take into account the existing GC_POINTER_IF etc. So the WE needs to be fixed before these changes go in. But the _IFDEF detection changes in the script can definitely go into a separate bug.
 
> - There needs to be more resilience here, gcmemberFieldTag
>   will miss GCMember lines that aren't formatted correctly (eg split
>   across two lines), that's a step backward from what we had.  If there's a
>   GCMember on a non-comment line we must require that the line is correctly
>   formatted so that we can parse it.

Agreed on more resilience. This is just the first pass and it successfully generates tracers with the current player code. 

> - What happens in the case of eg "GCMember<Blah> x, y;", which is legal?  Now
>   that I write that I realize that the old code does not handle that, and
>   most of us do not write code like that, but it'll be easier to write it now
>   that annotations are not required.

This wont get processed correctly and yes its not being used anywhere currently. Needs to be taken care in the script. 
I generated the tracers with the changes and without them and did not find any differences. 

> SR- but clearly going in the right direction.

Comment 6

[Tommy Reilly]

Comment on attachment 538364 [details] [diff] [review]
Initial patch

Canceling my review until a new patch addressing Lar's concerns exist.  Also note that people sometimes do GCMember <T> , especially if T is templated itself.  Goes along with more liberal whitespace handling of parsing GCMember < T > but we should also test against T having a template too to make sure we get the <> nesting right.

Comment 7

[Ruchi Lohani]

(In reply to comment #6)
> but we should also test against T having a template too to
> make sure we get the <> nesting right.

Tom, what do you mean by this? usually in case of nested template, I have noticed using GCMember< Templated_Type<T> > membername; 
Agreed that spaces between GCMember and '<' should be handled. I have that in the list of edge cases.

Comment 8

[Tommy Reilly]

Presumably to parse "GCMember<> name;" correctly to get at the "name" you need to make sure the regular expression doesn't stop at the first '>'.  If you have a greedy match and your RE includes name and the semi-colon you should be fine (modulo multiline decls).

Comment 9

[Ruchi Lohani]

Attachment: Included review comments and more changes

Summary of changes: 
1. Handled spaces between GCMember and '<'. Nested template wont cause a problem.
2. Handled multiple GCMembers (without any GC_POINTER annotation) on the same line
3. Handled GCMember extending to multiple lines
4. Handled GCMember as part of a single line comment 

I also think that we need to extend the singleLineCommentIndex approach to other field annotations. Currently, a GC_STRUCTURE or similar inside a // will be processed and we will generate code for not-existing member, though that will fail the build and force the dev to fix that. 

I changed the script such that the tracer generation will fail with an error if the GCMember declaration extends to more than one line. I found 2 such places in AVM and no cases in Player.

Comment 10

[Tommy Reilly]

Comment on attachment 539265 [details] [diff] [review]
Included review comments and more changes

Test code mods should be a separate patch and we can discuss whether they should be pushed or not (since there's no self tests for the trace generator I think it makes sense and would r+).   I'm pretty sure Lars will want an else instead of a continue in the handling of GC_POINTER case.  Looks good otherwise.

Comment 11

[Tommy Reilly]

Wait I mis-read the diff, I thought you added the newline to test your multi-line handling.   But you removed the newline because the script doesn't work w/ newlines.   I thought Lars was requesting we support that.   Lars, clarification?

Comment 12

[Ruchi Lohani]

(In reply to comment #11)
> Wait I mis-read the diff, I thought you added the newline to test your
> multi-line handling.   But you removed the newline because the script
> doesn't work w/ newlines.   I thought Lars was requesting we support that.  
> Lars, clarification?

No I did not experiment with handling newlines for multi-line GCMember declarations. I thought lars proposed we fail for those? 
From comment 4:
"If there's a GCMember on a non-comment line we must require that the line is correctly formatted so that we can parse it."

May be I misinterpreted what Lars proposed?

Comment 13

[Lars T Hansen]

(In reply to comment #12)
> (In reply to comment #11)
> > Wait I mis-read the diff, I thought you added the newline to test your
> > multi-line handling.   But you removed the newline because the script
> > doesn't work w/ newlines.   I thought Lars was requesting we support that.  
> > Lars, clarification?
> 
> No I did not experiment with handling newlines for multi-line GCMember
> declarations. I thought lars proposed we fail for those? 
> From comment 4:
> "If there's a GCMember on a non-comment line we must require that the line
> is correctly formatted so that we can parse it."
> 
> May be I misinterpreted what Lars proposed?

Ruchi right Tommy wrong.  I think we should keep it simple: if you use the string "GCMember" inside a GC_DATA_BEGIN/END section, and that word does appear inside a comment (block comments are a challenge here, so probably not supported), then that locks you into a certain format you must follow: template args and identifier on the same line as "GCMember", and only one name defined by the phrase.  Everything else is a signaled error (to avoid missing cases).  We can do better with Clang etc, but that's for later.

Comment 14

[Ruchi Lohani]

Another reminder to self: add more tests for these changes to core/ExactGCTests.h

Comment 15

[Lars T Hansen]

(In reply to comment #13)
> 
> ... if you use the
> string "GCMember" inside a GC_DATA_BEGIN/END section, and that word does
> appear inside a comment 

Should be "does not appear".

Comment 16

[Lars T Hansen]

Comment on attachment 539265 [details] [diff] [review]
Included review comments and more changes

(Preliminary review, I will do another pass soon)

The whole point of the use of indexOf is as a fast test to avoid firing up the regular expression engine for every line of input.  Thus the two line.replace clauses, though understandable, are not acceptable unless you can show no performance regression (I assure you I will be happy to be wrong, but the indexOf testing made its appearance because performance was bad).

At most you should probably do the replacing if eg indexOf("GCMember") >= 0 or indexOf("/*") >= 0, even then it might be better to wait until initial filtering has been performed.  Similarly it may be better to compute the singlelineCommentIndex later, after we've convinced ourselves the line is worth it.

(Performance is and will remain important for this script.)

Comment 17

[Ruchi Lohani]

(In reply to comment #16)
> Comment on attachment 539265 [details] [diff] [review] [review]
> Included review comments and more changes
> 
> (Preliminary review, I will do another pass soon)
> 
> The whole point of the use of indexOf is as a fast test to avoid firing up
> the regular expression engine for every line of input.  Thus the two
> line.replace clauses, though understandable, are not acceptable unless you
> can show no performance regression (I assure you I will be happy to be
> wrong, but the indexOf testing made its appearance because performance was
> bad).
> 
> At most you should probably do the replacing if eg indexOf("GCMember") >= 0
> or indexOf("/*") >= 0, even then it might be better to wait until initial
> filtering has been performed.  Similarly it may be better to compute the
> singlelineCommentIndex later, after we've convinced ourselves the line is
> worth it.
> 
> (Performance is and will remain important for this script.)

Good point. I will take a look from performance point of view. How did you measure performance when you worked on it initially?

Comment 18

[Lars T Hansen]

(In reply to comment #17)

> Good point. I will take a look from performance point of view. How did you
> measure performance when you worked on it initially?

I measured the time it took to rebuild the tracers for Tamarin.  The profiling code is still in the script, set profiling = true in the configuration section.

Comment 19

[Lars T Hansen]

Comment on attachment 539265 [details] [diff] [review]
Included review comments and more changes

gcPointerRegex should be lifted to the top level (it will be recompiled every time through the loop, not good).

Tommy's right, I don't like that "continue", an "else" would be stylistically more in keeping with the control flow in that part of the function.

Otherwise looks good.

Comment 20

[Ruchi Lohani]

Attachment: Patch v2

Comment 21

[Lars T Hansen]

Comment on attachment 539886 [details] [diff] [review]
Patch v2

This is fine but I'd like to request one more refinement (no review necessary).  In the cases flagged by the comment "Filters for GCMember edge cases", we're in the situation where we've seen "GCMember", and we're trying to filter out instances inside comments.  But what if it's GCMember alone by itself on some line, eg, if the code is written like this:

    GCMember
      <MyType> x;

This can happen when line breaks are inserted carelessly or by mistake and will still compile.  In this case the script should fail: that is, if the recomputation of gcmemberIndex (searching for "GCMember<") returns -1 then fail.

(Of course there might be reasons why that's not workable but I think we should try to flag obvious problems as well as we can.)

Comment 22

[Ruchi Lohani]

Attachment: Patch v3

Some more review comments incorporated. GCMember
                                            <SomemeClass> member;
will be flagged as a failure ( gcmember need to be on single line)

Comment 23

[Ruchi Lohani]

Attachment: Include test cases in the ExactGC tracer generation test file

Comment 24

[Ruchi Lohani]

Attachment: Documentation changes

Comment 25

[Tommy Reilly]

Comment on attachment 540935 [details] [diff] [review]
Documentation changes

< and > are preferrable to #60/#62.

Seems odd to me that there are no examples using GCMember in the cookbook!

Comment 26

[Ruchi Lohani]

Attachment: patch

Updated docs.

Comment 27

[Tommy Reilly]

Comment on attachment 545533 [details] [diff] [review]
patch

I'm pretty sure even Jamie Zawinski would dissapprove of using the blink tag here ;-)   Feel free to use them in a follow up email to Flash engineering explaining this change though!   +1 if the blink tag is removed

Comment 28

[Ruchi Lohani]

Attachment: Doc changes

Comment 29

[Tommy Reilly]

Comment on attachment 540933 [details] [diff] [review]
Patch v3

comments need a space "// comments"

extra whitespace added needs to go

if (cls != null) in pushGCmembers looks suspect, could it ever be null? consider var attr = [cls] instead of unshift

 gcmemberIndex = line.indexOf("GCMember<");
 if(gcmemberIndex == -1) {

Seems like an incomplete test of being spread across lines like what if the newline is after the <?  why not lookahead of semicolon and fail if you dont' find one?

Comment 30

[Ruchi Lohani]

Attachment: Patch v4

-Removed whitespaces
-Updated comments with a space between "//" and comment
-removed NULL check on cls in pushGCmembers

Comment 31

[Ruchi Lohani]

(In reply to comment #29)
> Comment on attachment 540933 [details] [diff] [review] [review]
> Patch v3

>  gcmemberIndex = line.indexOf("GCMember<");
>  if(gcmemberIndex == -1) {
> 
> Seems like an incomplete test of being spread across lines like what if the
> newline is after the <?  why not lookahead of semicolon and fail if you
> dont' find one?

If there is a newline after <, script will fail later as matchGCMemberFields  returns null.

Comment 32

[Ruchi Lohani]

Attachment: generated file changes

Do I need to request a superreview? Let me know.

Comment 33

[Tommy Reilly]

Comment on attachment 546571 [details] [diff] [review]
generated file changes

No reviews necessary for changes to machine generated files.  Changes should be folded into patch that made the changes necessary before pushing (keeping changes to generated files out of diffs for review is nice).

Comment 34

[Ruchi Lohani]

Attachment: Test changes

Fixed the test changes. Previous patch did not build.

Comment 35

[Tamarin Bot]

changeset: 6464:1ba0436a5531
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 660013: cleanup gcmember data member declarations in prepration to land the script changes (p=rulohani, r=treilly, sr=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/1ba0436a5531

Comment 36

[Tamarin Bot]

changeset: 6465:50f4e1643ea6
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 660013: GCMember-annotated members should not need GC_POINTER, it should be inferred by exactgc.as (p=rulohani, r=treilly, sr=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/50f4e1643ea6

Comment 37

[Tamarin Bot]

changeset: 6466:de129554bf17
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 660013 - GCMember-annotated members should not need GC_POINTER: doc changes (p=rulohani, r=treilly)

http://hg.mozilla.org/tamarin-redux/rev/de129554bf17

Comment 38

[Tamarin Bot]

changeset: 6484:0265fbd745ac
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 660013: Add test cases to ExactGCTests.h (r=treilly, sr=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/0265fbd745ac