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
