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

[Jim Graham]

Hi Mario,

I'm sorry that the communication path here is so thin.  I would have 
loved to have discussed this particular effort in more detail before you 
got into it very far.  Unfortunately, the end of June was a busy time 
for me and I didn't review and respond to your message on 6/24 as I 
should have.

I hate to dampen enthusiasm here, but if I had to prioritize the tasks 
you mentioned then I might have preferred one of the other tasks you had 
outlined first - mainly because, as I get to below, I might have 
recommended that you not do this part.

The fields have been working just fine so far as they are and this patch 
fixes no bugs in the code.  If we were instituting new policies for a 
given field that required an accessor then I could buy into adding one, 
but the changes here are not mandated by any policy changes under 
consideration.  And with respect to code cleanliness, private and public 
fields are not a big deal in internal code that doesn't have to promise 
backwards compatibility.  When we need to change a field, we do so and 
nothing breaks because there are no external dependencies - we own all 
of the code that needs to be updated.

Even more to the point, the sheer size of this patch is disruptive.  I 
don't know if there is much of a chance of a cut/paste error in all of 
it - perhaps you used an automated tool to make the changes and so the 
textual replacement is somewhat reliable, but at the very least there is 
now a bit of busy-work on the other end for someone to review the patch 
and verify that every "mumblex" and "mumbley" reference got changed into 
the corresponding "getMumbleX()" and "getMumbleY()" accessors.

I also actually would prefer to keep as many bare field references in 
the code as possible for other reasons.  There is nothing that paints a 
better picture as to the possible performance ramifications of a 
particular attribute fetch as a bare field reference.  If I am writing 
performance critical code then I know exactly how expensive "sg2d.pixel" 
  is going to be, but I have to wonder and investigate if 
"sg2d.getPixel()" is going to be enough of a problem to have to cache 
the value locally or not.  If everything looks like a method call, it 
becomes that much harder and more time consuming to write fast code and 
verify if any of the many patches that we review are going to affect 
performance or not.  To that end, I rather see the accessors as a 
problem rather than a solution.  This is more of an issue in 
performance-intensive code like the internals of a graphics engine than 
in application code.

In the end, I am very sorry that I didn't bring all of this up earlier 
in more detail.  I was time-pressured and wrote just a basic summary of 
where I thought it would make sense to spend time on this bug fix and 
now I feel that the appropriate response here is to say that I'd vote 
against this particular change, unfortunately and very much not happily 
after you've done all this work.  :-(

I am open to hearing how having accessors here might improve our 
development at this point, but my gut feel after having worked on this 
code for almost a decade is that we are better off without them.  Am I 
missing something?

And if you want to vent some frustration - fire away (privately 
hopefully)...

			...jim

Mario Torre wrote:
> Il 02/07/2009 17:07, Roman Kennke ha scritto:
>> Hi Mario,
>>
>>> Would you mind if I divide the patch in 2 or 3 slots?
>>
>> Actually this is a great idea! After completing each slot, you could
>> check with J2DBench if performance is actually affected or not.
>>
>>> I'll try to give you something already today, hopefully.
>>
>> Yay! I'm still waiting ;-)
> 
> Ugh, that one sucked time :)
> 
> Ok, so here is the webrew for the "make all fields private" part:
> 
> http://cr.openjdk.java.net/~neugens/100068/webrev.04/
> 
> Things to say:
> 
> 1. Looks like some fields are never accessed outside SunGraphics2D, no 
> accessor are provided for those.
> 
> 2. AffineTransform was a lot of fun to make correctly, because we need 
> both the "raw" transform and the the transform returned by getTransform, 
> because in some areas we need to know about clipping and device 
> coordinates, while in some other places we don't want to know about 
> that. I added a getRawTransform, I hope this name is meaningfull.
> 
> 3. I tested carefully, but I may have missed something, so I need some 
> help with this, it's a huge patch. I need help to test the windows fluff 
> especially.
> 
> 4. Everything is made final, but in some places we may still cache stuff 
> returned by the getters, I'll do this if this patch is ok and gets 
> committed.
> 
> 5. Just because I don't like to have 4 points :)
> 
> If this is ok, I'll move on the other things.
> 
> Cheers,
> Mario