Re: RFR (S) 8149835: StringConcatFactory should emit classes with the same package as the host class

[Aleksey Shipilev]

On 02/17/2016 07:10 PM, Alan Bateman wrote:
> 
> 
> On 17/02/2016 13:41, Aleksey Shipilev wrote:
>> :
>>
>> Alan, Mandy, would you like to review this as well? This should fix Jake
>> compatibility problems.
>>
>>
> Yes, I think this is okay although we'll need to look at the IMPL_LOOKUP
> usage when we get this to jake.

Thanks Alan and Mandy!

Lookup.IMPL_LOOKUP is used by LambdaMetafactory as well, so you would
need to see into all JDK BSMs.

-Aleksey

Re: RFR (S) 8149835: StringConcatFactory should emit classes with the same package as the host class

[Alan Bateman]

On 17/02/2016 13:41, Aleksey Shipilev wrote:

:

Alan, Mandy, would you like to review this as well? This should fix Jake
compatibility problems.


Yes, I think this is okay although we'll need to look at the IMPL_LOOKUP 
usage when we get this to jake.


-Alan

Re: RFR (S) 8149835: StringConcatFactory should emit classes with the same package as the host class

[Mandy Chung]

> On Feb 17, 2016, at 5:41 AM, Aleksey Shipilev  
> wrote:
> 
> On 02/17/2016 01:48 PM, Paul Sandoz wrote:
>>> On 17 Feb 2016, at 10:36, Aleksey Shipilev  
>>> wrote:
>>> Yes:
>>> http://cr.openjdk.java.net/~shade/8149835/webrev.jdk.02/
>>> http://cr.openjdk.java.net/~shade/8149835/webrev.langtools.01/
>>> 
>>> I have also changed to Lookup.IMPL_LOOKUP when looking up the method
>>> from U.defineAnonymousClass-loaded bytecode stub. This make the load
>>> sequence consistent with LambdaMetafactory.
>>> 
>>> It still passes JPRT, java/lang/String and jake build.
>>> 
>> 
>> +1
> 
> Thanks Paul!
> 
> Alan, Mandy, would you like to review this as well? This should fix Jake
> compatibility problems.

Looks okay to me.

Mandy

Re: RFR (S) 8149835: StringConcatFactory should emit classes with the same package as the host class

[Aleksey Shipilev]

On 02/17/2016 01:48 PM, Paul Sandoz wrote:
>> On 17 Feb 2016, at 10:36, Aleksey Shipilev  
>> wrote:
>> Yes:
>>  http://cr.openjdk.java.net/~shade/8149835/webrev.jdk.02/
>>  http://cr.openjdk.java.net/~shade/8149835/webrev.langtools.01/
>>
>> I have also changed to Lookup.IMPL_LOOKUP when looking up the method
>> from U.defineAnonymousClass-loaded bytecode stub. This make the load
>> sequence consistent with LambdaMetafactory.
>>
>> It still passes JPRT, java/lang/String and jake build.
>>
> 
> +1

Thanks Paul!

Alan, Mandy, would you like to review this as well? This should fix Jake
compatibility problems.

Cheers,
-Aleksey

Re: RFR (S) 8149835: StringConcatFactory should emit classes with the same package as the host class

[Paul Sandoz]

> On 17 Feb 2016, at 10:36, Aleksey Shipilev  
> wrote:
> 
> On 02/16/2016 11:59 PM, Andrej Golovnin wrote:
>> Hi Aleksey,
>> 
>>> http://cr.openjdk.java.net/~shade/8149835/webrev.jdk.01/
>> 
>> 701 return (pkg != null ? pkg.getName().replace(".", 
>> "/") + "/" : "") + "Stubs$$StringConcat";
>> 702 } else {
>> 703 return hostClass.getName().replace(".", "/") + 
>> "$$StringConcat”;
>> 
>> Maybe you should use here the character based String#replace()-method as it 
>> is faster and
>> does not produce as much garbage as the CharSequence based method does.
> 
> Yes:
>  http://cr.openjdk.java.net/~shade/8149835/webrev.jdk.02/
>  http://cr.openjdk.java.net/~shade/8149835/webrev.langtools.01/
> 
> I have also changed to Lookup.IMPL_LOOKUP when looking up the method
> from U.defineAnonymousClass-loaded bytecode stub. This make the load
> sequence consistent with LambdaMetafactory.
> 
> It still passes JPRT, java/lang/String and jake build.
> 

+1

Paul.

Re: RFR (S) 8149835: StringConcatFactory should emit classes with the same package as the host class

[Aleksey Shipilev]

On 02/16/2016 11:59 PM, Andrej Golovnin wrote:
> Hi Aleksey,
> 
>>  http://cr.openjdk.java.net/~shade/8149835/webrev.jdk.01/
> 
> 701 return (pkg != null ? pkg.getName().replace(".", "/") 
> + "/" : "") + "Stubs$$StringConcat";
> 702 } else {
> 703 return hostClass.getName().replace(".", "/") + 
> "$$StringConcat”;
> 
> Maybe you should use here the character based String#replace()-method as it 
> is faster and
> does not produce as much garbage as the CharSequence based method does.

Yes:
  http://cr.openjdk.java.net/~shade/8149835/webrev.jdk.02/
  http://cr.openjdk.java.net/~shade/8149835/webrev.langtools.01/

I have also changed to Lookup.IMPL_LOOKUP when looking up the method
from U.defineAnonymousClass-loaded bytecode stub. This make the load
sequence consistent with LambdaMetafactory.

It still passes JPRT, java/lang/String and jake build.

Cheers,
-Aleksey

Re: RFR (S) 8149835: StringConcatFactory should emit classes with the same package as the host class

[Andrej Golovnin]

Hi Aleksey,

>  http://cr.openjdk.java.net/~shade/8149835/webrev.jdk.01/

701 return (pkg != null ? pkg.getName().replace(".", "/") + 
"/" : "") + "Stubs$$StringConcat";
702 } else {
703 return hostClass.getName().replace(".", "/") + 
"$$StringConcat”;

Maybe you should use here the character based String#replace()-method as it is 
faster and
does not produce as much garbage as the CharSequence based method does.

Best regards,
Andrej Golovnin

RFR (S) 8149835: StringConcatFactory should emit classes with the same package as the host class

[Aleksey Shipilev]

Hi,

Please review a StringConcatFactory fix that makes generated String
concat stubs to be named "$$StringConcat/", instead of
"java/lang/String$$Concat/":
  https://bugs.openjdk.java.net/browse/JDK-8149835

Among other things, this helps to dodge the failure caught by stricter
access controls in Jake (JDK-8149165), and hopefully caters for future
stricter Unsafe.defineAnonymousClass access controls (JDK-8058575).

Webrevs:
  http://cr.openjdk.java.net/~shade/8149835/webrev.jdk.01/
  http://cr.openjdk.java.net/~shade/8149835/webrev.langtools.01/

Testing: JPRT, java/lang/String; + copying SCF code to jake forest and
running with it.

Cheers,
-Aleksey