[OpenJDK 2D-Dev] RFR: 8038875: Remove use of ServiceLoader in finding class implementing sun.java2d.pipe. RenderingEngine

[Phil Race]

On 5/7/14 12:26 PM, Mandy Chung wrote:
> On 5/7/2014 11:26 AM, Phil Race wrote:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8038875
>> http://cr.openjdk.java.net/~prr/8038875/
>>
>
> Thanks for taking this on.  I have some minor comments:
>
> line 129: if sun.java2d.renderer is set,  it finds a custom 
> RenderingEngine from the bootclasspath (as the current class loader is 
> the bootstrap class loader).   On the other hand, in the original 
> implementation, the service loader loads from the extension class 
> loader.  I'm not proposing to use a different class loader since this 
> is not a supported way to plugin a custom rendering engine. Just to 
> mention it to call out the difference.

Probably it should always have been the bootstrap class loader
since this is a test mechanism for replacing an internal body of code.
But permissions are the same in both cases, aren't they ?

>
> You may consider to use the 3-arg version of Class.forName and not to 
> initialize the class.  This is called within a doPrivileged block and 
> it's generally a good convention to invoke the class initialization of 
> the specified class (from the system property) outside the 
> doPrivileged block.  Static analysis tool may consider catching the 
> 1-arg version of Class.forName and flag it as a warning.
I suppose could do that but I'm not sure how much it matters besides 
shutting up a tool
given that
a) its going to be on the bootclasspath
b) its got to be specified by a property that can only be set on the 
command line
or privileged code
>
> line 134-140: PiscesRenderingEngine is always present, right? Would it 
> be better just to return "new PiscesRenderingEngine()"?
It looked as if it is always there, although I'm not sure if that was 
intended or is just
the way the build happened to work. So I'd prefer to leave it as  ..

-phil.

>
> otherwise, looks good.
> Mandy
>
>> This is in support of modularisation (ie jigsaw). After the fix 
>> alternate implementations
>> are loaded using Class.forName() as specified by the system property
>> sun.java2d.renderer. Note that only the built-in ones are actually 
>> supported
>> The mechanism is just for testing.
>>
>> BTW since it happens that Oracle JDK includes the pisces classes as well
>> as the closed source ductus I note that its possible to compare the 
>> performance
>> or other characteristics easily in a closed build which has both by 
>> specifying
>> to use pisces instead of ductus.
>>
>> There's no regression test associated with this. I tested open & 
>> closed builds
>> (created for all platforms) with standard demos.
>>
>> -phil.
>