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

[Mandy Chung]

On 5/7/2014 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.
>
> 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.

The class will need to be initialized in the newInstance() call. The 
above comment might not apply.   Is the doPrivileged block needed for 
the RenderingEngine instantiation?  I was wondering if it was needed for 
the service loader only or not.

Mandy

>
> line 134-140: PiscesRenderingEngine is always present, right? Would it 
> be better just to return "new PiscesRenderingEngine()"?
>
> 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.
>