Re: RFR: 8167002: JAXP schema validator: Use HashSet instead of ArrayList for tracking XML IDs

[Joe Wang]

+1, great improvement!

Best,
Joe

On 10/3/16, 7:28 AM, Daniel Fuchs wrote:

Hi Martin,

This looks good to me.

best regards,

-- daniel

On 01/10/16 01:41, Martin Buchholz wrote:

https://bugs.openjdk.java.net/browse/JDK-8167002
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/xml-id-validation-by-hash/ 

Re: RFR: 8167002: JAXP schema validator: Use HashSet instead of ArrayList for tracking XML IDs

[Daniel Fuchs]

Hi Martin,

This looks good to me.

best regards,

-- daniel

On 01/10/16 01:41, Martin Buchholz wrote:

https://bugs.openjdk.java.net/browse/JDK-8167002
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/xml-id-validation-by-hash/

RE: RFR: 8167002: JAXP schema validator: Use HashSet instead of ArrayList for tracking XML IDs

[Langer, Christoph]

Hi Martin,

this also looks like a good catch to me, +1.

When you push, could you insert the copyright block from, e.g. 
src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/XML11DocumentScannerImpl.java
 at the place where you currently find the " reserved comment block ",  first 4 
lines? The XML copyright headers get cleaned up as they are touched...

Thanks and best regards
Christoph
 
> -Original Message-
> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf
> Of Martin Buchholz
> Sent: Samstag, 1. Oktober 2016 18:55
> To: Claes Redestad <claes.redes...@oracle.com>
> Cc: core-libs-dev <core-libs-dev@openjdk.java.net>
> Subject: Re: RFR: 8167002: JAXP schema validator: Use HashSet instead of
> ArrayList for tracking XML IDs
> 
> On Sat, Oct 1, 2016 at 5:45 AM, Claes Redestad <claes.redes...@oracle.com>
> wrote:
> 
> >
> > On 2016-10-01 02:41, Martin Buchholz wrote:
> >
> >> https://bugs.openjdk.java.net/browse/JDK-8167002
> >> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/xml-id-v
> >> alidation-by-hash/
> >>
> >>
> > +1, but I have to ask what the intended benefit of writing:
> >
> >   ((fIds != null) ? fIds : (fIds = new HashSet<>())).add(name);
> >
> > rather than keeping the pre-existing pattern:
> >
> >   if (fIds == null) fIds = new HashSet<>();
> >   fIds.add(name);
> >
> > If this is about bytecode optimization to help with inlining or such,
> > the latter actually generate a more compact method (14 vs 16 bytecodes).
> 
> 
> The intent was (by instinct) to not code a re-read of a field, but ... yeah
>  that wasn't achieved.
> That could be done correctly using
> 
> public void addId(String name) {
> HashSet x;
> if ((x = fIds) == null) fIds = x = new HashSet<>();
> x.add(name);
> }
> 
> but I'll just revert to
> if (fIds == null) fIds = new HashSet<>();
> fIds.add(name);

Re: RFR: 8167002: JAXP schema validator: Use HashSet instead of ArrayList for tracking XML IDs

[Claes Redestad]

On 2016-10-01 18:54, Martin Buchholz wrote:


On Sat, Oct 1, 2016 at 5:45 AM, Claes Redestad
> wrote:


On 2016-10-01 02:41, Martin Buchholz wrote:

https://bugs.openjdk.java.net/browse/JDK-8167002


http://cr.openjdk.java.net/~martin/webrevs/openjdk9/xml-id-validation-by-hash/




+1, but I have to ask what the intended benefit of writing:

   ((fIds != null) ? fIds : (fIds = new HashSet<>())).add(name);

rather than keeping the pre-existing pattern:

   if (fIds == null) fIds = new HashSet<>();
   fIds.add(name);

If this is about bytecode optimization to help with inlining or such,
the latter actually generate a more compact method (14 vs 16 bytecodes).


The intent was (by instinct) to not code a re-read of a field, but ...
yeah  that wasn't achieved.
That could be done correctly using

 public void addId(String name) {
 HashSet x;
 if ((x = fIds) == null) fIds = x = new HashSet<>();
 x.add(name);
 }

but I'll just revert to
 if (fIds == null) fIds = new HashSet<>();
 fIds.add(name);



Ok, thanks for the explanation!

Re: RFR: 8167002: JAXP schema validator: Use HashSet instead of ArrayList for tracking XML IDs

[Martin Buchholz]

On Sat, Oct 1, 2016 at 5:45 AM, Claes Redestad 
wrote:

>
> On 2016-10-01 02:41, Martin Buchholz wrote:
>
>> https://bugs.openjdk.java.net/browse/JDK-8167002
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/xml-id-v
>> alidation-by-hash/
>>
>>
> +1, but I have to ask what the intended benefit of writing:
>
>   ((fIds != null) ? fIds : (fIds = new HashSet<>())).add(name);
>
> rather than keeping the pre-existing pattern:
>
>   if (fIds == null) fIds = new HashSet<>();
>   fIds.add(name);
>
> If this is about bytecode optimization to help with inlining or such,
> the latter actually generate a more compact method (14 vs 16 bytecodes).


The intent was (by instinct) to not code a re-read of a field, but ... yeah
 that wasn't achieved.
That could be done correctly using

public void addId(String name) {
HashSet x;
if ((x = fIds) == null) fIds = x = new HashSet<>();
x.add(name);
}

but I'll just revert to
if (fIds == null) fIds = new HashSet<>();
fIds.add(name);

Re: RFR: 8167002: JAXP schema validator: Use HashSet instead of ArrayList for tracking XML IDs

[Lance Andersen]

This looks fine to me also


> On Sep 30, 2016, at 8:41 PM, Martin Buchholz  wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8167002
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/xml-id-validation-by-hash/

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: 8167002: JAXP schema validator: Use HashSet instead of ArrayList for tracking XML IDs

[Claes Redestad]

On 2016-10-01 02:41, Martin Buchholz wrote:

https://bugs.openjdk.java.net/browse/JDK-8167002
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/xml-id-validation-by-hash/



+1, but I have to ask what the intended benefit of writing:

  ((fIds != null) ? fIds : (fIds = new HashSet<>())).add(name);

rather than keeping the pre-existing pattern:

  if (fIds == null) fIds = new HashSet<>();
  fIds.add(name);

If this is about bytecode optimization to help with inlining or such,
the latter actually generate a more compact method (14 vs 16 bytecodes).

/Claes

RFR: 8167002: JAXP schema validator: Use HashSet instead of ArrayList for tracking XML IDs

[Martin Buchholz]

https://bugs.openjdk.java.net/browse/JDK-8167002
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/xml-id-validation-by-hash/