[OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised

[Jim Graham]

Hi Mario,

The changes look safe, but I wanted to bring up the following issues:

- There are a dozen or so fields in SG2D that are commonly accessed 
everywhere in the pipelines including loops.  These changes introduce an 
accessor for loops, but that now looks out of place with no accessors 
for any of the other fields that get accessed directly.  Personally the 
lack of accessors hasn't bothered me, particularly since this is 
performance sensitive code and accessors can sap performance by nickels 
and dimes if you don't implement them correctly - to wit:

- Accessors can be inlined if they are final, but these aren't.  As it 
turns out, SG2D itself is final and so I believe that is enough for them 
to be inlined, but I tend to make methods final as well to underscore 
that they are intended to be inlined, and also in case we eventually 
decide to make the class non-final.  On the other hand, we haven't 
really bothered with accessors in the first place in this code to avoid 
the code bloat and the potential points of heuristic failure.

- I agree that it would be nice to ask the pipes if they need loops, I'm 
not sure why your solution didn't work, but I prefer that to making them 
a side effect of getTextPipe() since it makes it harder to know when the 
code you are editing needs to get the loops or not, and it makes it hard 
to see where they come from when editing the many validatePipe() methods.

			...jim

Mario Torre wrote:
> Il 18/06/2009 21:52, Jim Graham ha scritto:
> 
>> One solution would be to always set the loops for the validations that
>> install one of these pipes. That could have potential performance
>> impact, but it would be no worse than the validation sequences that
>> already set the loops every time so I don't think it would be serious,
>> or even measurable.
> 
> Hi Jim,
> 
> I'm trying this approach.
> 
> Basically, I just set the loops to null now in invalidate and validate 
> them back in validatePipe:
> 
> http://cr.openjdk.java.net/~neugens/100068/webrev.03/
> 
> What I do is to get valid Loops when we call getTextPipe, but other than 
> that, the behavior is basically unchanged. I tried with various 
> applications, including NetBeans, the Java2D demo and the FontTest app, 
> playing around with the text configurations (AA, LCD, size, Glyphs etc.. 
> even output to a PNG file) and looks good, but I may miss something of 
> course.
> 
> I moved to a 64 bit machine, I don't think this makes any difference in 
> this case, but maybe it's a good thing to say.
> 
> I would like to see a method in the pipelines that does something like:
> 
> boolean mustInitialiseLoopsBecauseWeReallyWantToUseThemGranted()
> 
> or so, that we may call at the end of the validatePipe method, but when 
> I tried this I got a funny StackOverflowException, maybe I did something 
> wrong, but looks like it's not so easy to change this code and still 
> make it behave correctly, so I would like to go deeper in this issue 
> even if you think we can close the bug (of course if the proposed fix is 
> evaluated as complete).
> 
> Cheers,
> Mario