Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno mar, 16/06/2009 alle 16.18 -0700, Jim Graham ha scritto: > Ah, I think I see the problem. Phil can chime in here if I'm wrong. Yay, glad it helped :) > Text now uses loops in most cases, but many of the validate methods > assume that they don't need the loops. I don't think that used to be > the case, but it changed as a result of the LCD text loop work. It used > to be that AA used the alphafill field of SG2D and not the loops, and it > still does for most rendering. But now text rendering will almost > always go through the loops and it is only working because of that call > to getRenderLoops() in the constructor (and the fact that we never set > it back to null. That makes completely sense to me. > I'd like to see invalidate() set the loops to null and see how far we > get - I'll bet that we'd get NPEs all over the place, especially with AA > rendering. I'll try that tomorrow. I can also help with the testing, I would like to see those changes in the main code base at some point, and they will be surely useful for Cacio too. > In the long run, this is another straw on the camel's back as far as > rethinking the validation framework. It's been tweaked and hacked quite > a lot over the past 10 years and so there are a lot of issues that arise > like this that aren't readily obvious from the code. I don't think the > camel's back is broken yet, but it is becoming more and more confusing > for new engineers to start tinkering with it without seeing things break > from seemingly innocuous changes. :-( Eh... I've seen this in the FontManager code too, and in Classpath we had similar problems for some of the most obscure things too, that's an obvious consequence of code that has to preserve backward forever-compatibility, but I have to say that the Java2D code does some really cool things in many places :) > And Phil might want to chime in with a description of the new > requirements on loops in light of the post-LCD text work... Phil? That would be interesting :) Cheers (and good night!), Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/ -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Ah, I think I see the problem. Phil can chime in here if I'm wrong.
Text now uses loops in most cases, but many of the validate methods
assume that they don't need the loops. I don't think that used to be
the case, but it changed as a result of the LCD text loop work. It used
to be that AA used the alphafill field of SG2D and not the loops, and it
still does for most rendering. But now text rendering will almost
always go through the loops and it is only working because of that call
to getRenderLoops() in the constructor (and the fact that we never set
it back to null.
I'd like to see invalidate() set the loops to null and see how far we
get - I'll bet that we'd get NPEs all over the place, especially with AA
rendering.
In the long run, this is another straw on the camel's back as far as
rethinking the validation framework. It's been tweaked and hacked quite
a lot over the past 10 years and so there are a lot of issues that arise
like this that aren't readily obvious from the code. I don't think the
camel's back is broken yet, but it is becoming more and more confusing
for new engineers to start tinkering with it without seeing things break
from seemingly innocuous changes. :-(
For now, I'm wary of removing that call without a lot of testing. I
think putting a "loops=null" line in invalidatePipe() might accelerate
some of that testing, though.
And Phil might want to chime in with a description of the new
requirements on loops in light of the post-LCD text work... Phil?
...jim
Mario Torre wrote:
Il giorno lun, 15/06/2009 alle 13.37 -0700, Jim Graham ha scritto:
Hi Mario,
How are the drawGlyphList methods called when the loops is null? I ask
because they are only ever installed on the SunGraphics2D object by
virtue of a call to validatePipe() and all calls to validatePipe()
should set the loops. So, there is a bug somewhere that causes these
loops to be installed without a full validate process.
Hi Jim,
So, I spent some time today (sorry for the delay, I had to find some
free time slot for that and had to make a cake for my girl friend, which
was the most difficult part :), and I debugged the Java2D demo with just
the non initialised loops (so, no checks for null loops anywhere else in
the code).
The Demo starts fine, but after some point I get the error attached in
this mail.
As I said in the email you quoted. All calls from pipelines
(GlyphListLoopPipe and AATextRenderer are both pipeline objects) should
be safe because all calls to validatePipe() should set the loops.
I see that validatePipe is indeed called always, but sometimes the path
that is chosen doesn't validate the rendering loop. I've seen that
most of the time this is ok, because the loops are not used.
I guess this is the case for all the various text rendering (LCD or AA)
in swing components, for example (is this correct?).
Having said that I note that there are some pipelines that do not
require loops and it would be OK for a call to validate that only uses
such "non-loop-based" pipelines to leave the loops field uninitialized,
but all validate calls which use loop-based pipes must update the loops
field.
Exactly. I'm not deep into the code yet to distinguish when they are
needed or not, but...
Eliminating all of those uses of loops we then fall into the case where
the usage in checkFontInfo is the only usage that does not occur from a
pipeline...
...this is exactly the case, putting a null check here solves the NPE.
For what I can see, at some point some component needs to paint to an
offscreen surface (I don't think the offscreen surface is special,
I think it's the application code that drives the bug, but anyway...).
This is the background image from the Java2D Intro pane, I think the
other pane do something similar. Once the SunGraphics2d object is created,
some redering hints are set. This is the application code:
g2.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
That is, turn on antialiasing, then:
g2.clearRect(0, 0, d.width, d.height);
Now what? This of course goes through validatePipe:
The first two if/else statements fall through but not this
(SurfaceData, line 535 in OpenJDK):
} else if (sg2d.antialiasHint == SunHints.INTVAL_ANTIALIAS_ON) {
This guy sets the pipes but doesn't set the RederingLoops.
Then, again application code:
scene.render(d.width, d.height, g2);
Which after few loops finally renders the string on screen,
which causes the crash.
So, in other words, everything goes fine until we draw text with
the AA redering hints turned on.
Of course, before it was not failing because of the rendering loops
were initialised in the constructor.
I'm not sure if we need to check for a null loops at the beginning
of validatePipe or in checkFontoInfo. Maybe we can save something if we
use checkFontInfo but I'm not so sure.
I hope this helps,
Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno lun, 15/06/2009 alle 13.37 -0700, Jim Graham ha scritto:
> Hi Mario,
>
> How are the drawGlyphList methods called when the loops is null? I ask
> because they are only ever installed on the SunGraphics2D object by
> virtue of a call to validatePipe() and all calls to validatePipe()
> should set the loops. So, there is a bug somewhere that causes these
> loops to be installed without a full validate process.
Hi Jim,
So, I spent some time today (sorry for the delay, I had to find some
free time slot for that and had to make a cake for my girl friend, which
was the most difficult part :), and I debugged the Java2D demo with just
the non initialised loops (so, no checks for null loops anywhere else in
the code).
The Demo starts fine, but after some point I get the error attached in
this mail.
> As I said in the email you quoted. All calls from pipelines
> (GlyphListLoopPipe and AATextRenderer are both pipeline objects) should
> be safe because all calls to validatePipe() should set the loops.
I see that validatePipe is indeed called always, but sometimes the path
that is chosen doesn't validate the rendering loop. I've seen that
most of the time this is ok, because the loops are not used.
I guess this is the case for all the various text rendering (LCD or AA)
in swing components, for example (is this correct?).
> Having said that I note that there are some pipelines that do not
> require loops and it would be OK for a call to validate that only uses
> such "non-loop-based" pipelines to leave the loops field uninitialized,
> but all validate calls which use loop-based pipes must update the loops
> field.
Exactly. I'm not deep into the code yet to distinguish when they are
needed or not, but...
> Eliminating all of those uses of loops we then fall into the case where
> the usage in checkFontInfo is the only usage that does not occur from a
> pipeline...
...this is exactly the case, putting a null check here solves the NPE.
For what I can see, at some point some component needs to paint to an
offscreen surface (I don't think the offscreen surface is special,
I think it's the application code that drives the bug, but anyway...).
This is the background image from the Java2D Intro pane, I think the
other pane do something similar. Once the SunGraphics2d object is created,
some redering hints are set. This is the application code:
g2.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
That is, turn on antialiasing, then:
g2.clearRect(0, 0, d.width, d.height);
Now what? This of course goes through validatePipe:
The first two if/else statements fall through but not this
(SurfaceData, line 535 in OpenJDK):
} else if (sg2d.antialiasHint == SunHints.INTVAL_ANTIALIAS_ON) {
This guy sets the pipes but doesn't set the RederingLoops.
Then, again application code:
scene.render(d.width, d.height, g2);
Which after few loops finally renders the string on screen,
which causes the crash.
So, in other words, everything goes fine until we draw text with
the AA redering hints turned on.
Of course, before it was not failing because of the rendering loops
were initialised in the constructor.
I'm not sure if we need to check for a null loops at the beginning
of validatePipe or in checkFontoInfo. Maybe we can save something if we
use checkFontInfo but I'm not so sure.
I hope this helps,
Mario
--
Mario Torre, Software Developer, http://www.jroller.com/neugens/
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com * Tel: +49-721-663 968-44
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF
USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim
Geschäftsführer: Dr. James J. Hunt
Please, support open standards:
http://endsoftpatents.org/
--
Mario Torre, Software Developer, http://www.jroller.com/neugens/
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com * Tel: +49-721-663 968-44
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF
USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim
Geschäftsführer: Dr. James J. Hunt
Please, support open standards:
http://endsoftpatents.org/
In various place after some time the demo is started or when selecting the
ArcsCurves panel:
In Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
at sun.java2d.pipe.AATextRenderer.drawGlyphList(AATextRenderer.java:40)
at sun.java2d.pipe.GlyphListPipe.drawString(GlyphListPipe.java:72)
at sun.java2d.SunGraphics2D.drawString(SunGraphics2D.java:2808)
at demos.Arcs_Curves.Arcs.drawDemo(Arcs.java:128)
at DemoSurface.paint(DemoSurface.java:303)
at javax.swing.JComponent.paintToOffscreen(JCompone
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Not necessarily. The demo serves to test a wide variety of 2D functionality all in one place. It has worked very well to point out real bugs in the past that we have fixed, though it does have some questionable practices inside it that can turn up false bugs and I believe its abuse of threading is one such practice. Send me (off-line) the full stack traces of the failures you were seeing and I'll check and see if they occur in areas where it might be trying to do graphics from multiple threads at once... ...jim Mario Torre wrote: Il giorno mar, 16/06/2009 alle 12.08 -0700, Jim Graham ha scritto: It's probably also a good place to let slip that a certain demo that was mentioned previously in the thread has always been considered "poorly written" and I wouldn't put it past it to be violating the threading design goals we have. But I would never come right out and say something like that since it serves as a good "make sure even kooky things continue to work" telltale... [goes back to looking nonchalant] Ugh... don't tell me I'm debugging with a wrong test case... :S Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno mar, 16/06/2009 alle 12.08 -0700, Jim Graham ha scritto: > It's probably also a good place to let slip that a certain demo that was > mentioned previously in the thread has always been considered "poorly > written" and I wouldn't put it past it to be violating the threading > design goals we have. But I would never come right out and say > something like that since it serves as a good "make sure even kooky > things continue to work" telltale... [goes back to looking nonchalant] > Ugh... don't tell me I'm debugging with a wrong test case... :S Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
I should clarify that what I meant by "doesn't corrupt anything" is that the world won't come to an end or the system crash. It's protected against harm from MT, but it doesn't support MT. And another way of stating it is that we won't consider race condition bugs when it is used in an MT fashion, but we would consider fixing bugs that cause it to crash or that cause a G2D to latch onto a specific thread. It's probably also a good place to let slip that a certain demo that was mentioned previously in the thread has always been considered "poorly written" and I wouldn't put it past it to be violating the threading design goals we have. But I would never come right out and say something like that since it serves as a good "make sure even kooky things continue to work" telltale... [goes back to looking nonchalant] ...jim Jim Graham wrote: My design goal was "doesn't corrupt anything if run from multiple threads" (in other words don't require synchronization in any common places just to keep it functional and don't require it to be used from a particular thread), but only correct behavior if run from 1 thread at a time. In other words, it can be used by multiple threads in sequence, but not simultaneously. ...jim
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
My design goal was "doesn't corrupt anything if run from multiple threads" (in other words don't require synchronization in any common places just to keep it functional and don't require it to be used from a particular thread), but only correct behavior if run from 1 thread at a time. In other words, it can be used by multiple threads in sequence, but not simultaneously. ...jim Roman Kennke wrote: Hi there, first of all, let me say that - especially in the light of this thread, I support Mario's change (removal of this one line from the constructor) for the following reasons: - It should not be necessary as you say, this field should always be initialized before use by validatePipe(). - If it's not, it's most likely a bug. - Therefore this line is only good for hiding bugs. One thing is not quite clear to me: - A race condition where thread A is validating the pipeline and installs the pipeline objects but hasn't yet reached the code to install the loops while thread B starts rendering using that SG2D thus invoking an operation on a partially initialized pipeline - in this case the NPE is appropriate and allowed since we don't support multi-threaded use of the Graphics2D objects. I was always under the assumption, that Java2D should be thread safe. And in many places we make sure it is, e.g. in the SurfaceData objects. But now you say, it isn't. So what is the real thing about Java2D and thread safety. Is it only supposed to be thread safe when each thread gets its own Graphics2D object? I think I remember back in the GNU Classpath days we had a test case that shared a Graphics (no Graphics2D back then..) object between threads and was supposed to be ok, but I might be wrong here... Can you enlighten me what is the situation w/ Java2D and thread safety? / Roman
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi there, first of all, let me say that - especially in the light of this thread, I support Mario's change (removal of this one line from the constructor) for the following reasons: - It should not be necessary as you say, this field should always be initialized before use by validatePipe(). - If it's not, it's most likely a bug. - Therefore this line is only good for hiding bugs. One thing is not quite clear to me: - A race condition where thread A is validating the pipeline and installs the pipeline objects but hasn't yet reached the code to install the loops while thread B starts rendering using that SG2D thus invoking an operation on a partially initialized pipeline - in this case the NPE is appropriate and allowed since we don't support multi-threaded use of the Graphics2D objects. I was always under the assumption, that Java2D should be thread safe. And in many places we make sure it is, e.g. in the SurfaceData objects. But now you say, it isn't. So what is the real thing about Java2D and thread safety. Is it only supposed to be thread safe when each thread gets its own Graphics2D object? I think I remember back in the GNU Classpath days we had a test case that shared a Graphics (no Graphics2D back then..) object between threads and was supposed to be ok, but I might be wrong here... Can you enlighten me what is the situation w/ Java2D and thread safety? / Roman
