Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Maarten Coene-2
Converting an Enumeration to a List just for iterating it doesn't seem performance and memory wise a good idea to me.
Maarten

      Van: "[hidden email]" <[hidden email]>
 Aan: [hidden email]
 Verzonden: woensdag 16 mei 19:13 2018
 Onderwerp: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration
   
Repository: ant
Updated Branches:
  refs/heads/master ac35c0014 -> 070c3bc86


http://git-wip-us.apache.org/repos/asf/ant/blob/070c3bc8/src/main/org/apache/tools/ant/types/ZipScanner.java
----------------------------------------------------------------------
diff --git a/src/main/org/apache/tools/ant/types/ZipScanner.java b/src/main/org/apache/tools/ant/types/ZipScanner.java
index a3df040..5667159 100644
--- a/src/main/org/apache/tools/ant/types/ZipScanner.java
+++ b/src/main/org/apache/tools/ant/types/ZipScanner.java
@@ -20,7 +20,7 @@ package org.apache.tools.ant.types;
 
 import java.io.File;
 import java.io.IOException;
-import java.util.Enumeration;
+import java.util.Collections;
 import java.util.Map;
 import java.util.zip.ZipException;
 
@@ -62,10 +62,7 @@ public class ZipScanner extends ArchiveScanner {
                "Only file provider resources are supported"));
 
        try (ZipFile zf = new ZipFile(srcFile, encoding)) {
-
-            Enumeration<ZipEntry> e = zf.getEntries();
-            while (e.hasMoreElements()) {
-                ZipEntry entry = e.nextElement();
+            for (ZipEntry entry : Collections.list(zf.getEntries())) {
                Resource r = new ZipResource(srcFile, encoding, entry);
                String name = entry.getName();
                if (entry.isDirectory()) {


   
Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Jaikiran Pai
I agree. Especially when it's being done on something like for archive
entries which can betoo many depending on the archive that is being
dealt with.

-Jaikiran


On 17/05/18 12:04 PM, Maarten Coene wrote:

> Converting an Enumeration to a List just for iterating it doesn't seem performance and memory wise a good idea to me.
> Maarten
>
>        Van: "[hidden email]" <[hidden email]>
>   Aan: [hidden email]
>   Verzonden: woensdag 16 mei 19:13 2018
>   Onderwerp: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration
>    
> Repository: ant
> Updated Branches:
>    refs/heads/master ac35c0014 -> 070c3bc86
>
>
> http://git-wip-us.apache.org/repos/asf/ant/blob/070c3bc8/src/main/org/apache/tools/ant/types/ZipScanner.java
> ----------------------------------------------------------------------
> diff --git a/src/main/org/apache/tools/ant/types/ZipScanner.java b/src/main/org/apache/tools/ant/types/ZipScanner.java
> index a3df040..5667159 100644
> --- a/src/main/org/apache/tools/ant/types/ZipScanner.java
> +++ b/src/main/org/apache/tools/ant/types/ZipScanner.java
> @@ -20,7 +20,7 @@ package org.apache.tools.ant.types;
>  
>   import java.io.File;
>   import java.io.IOException;
> -import java.util.Enumeration;
> +import java.util.Collections;
>   import java.util.Map;
>   import java.util.zip.ZipException;
>  
> @@ -62,10 +62,7 @@ public class ZipScanner extends ArchiveScanner {
>                  "Only file provider resources are supported"));
>  
>          try (ZipFile zf = new ZipFile(srcFile, encoding)) {
> -
> -            Enumeration<ZipEntry> e = zf.getEntries();
> -            while (e.hasMoreElements()) {
> -                ZipEntry entry = e.nextElement();
> +            for (ZipEntry entry : Collections.list(zf.getEntries())) {
>                  Resource r = new ZipResource(srcFile, encoding, entry);
>                  String name = entry.getName();
>                  if (entry.isDirectory()) {
>
>
>    


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Gintautas Grigelionis
Thanks for reviewing, I hope Spliterators will do a better job.

Gintas

2018-05-17 8:37 GMT+02:00 Jaikiran Pai <[hidden email]>:

> I agree. Especially when it's being done on something like for archive
> entries which can betoo many depending on the archive that is being dealt
> with.
>
> -Jaikiran
>
>
>
> On 17/05/18 12:04 PM, Maarten Coene wrote:
>
>> Converting an Enumeration to a List just for iterating it doesn't seem
>> performance and memory wise a good idea to me.
>> Maarten
>>
>>        Van: "[hidden email]" <[hidden email]>
>>   Aan: [hidden email]
>>   Verzonden: woensdag 16 mei 19:13 2018
>>   Onderwerp: [1/2] ant git commit: Deprecate CollectionUtils and
>> Enumerations; reduce explicit use of Enumeration
>>     Repository: ant
>> Updated Branches:
>>    refs/heads/master ac35c0014 -> 070c3bc86
>>
>>
>> http://git-wip-us.apache.org/repos/asf/ant/blob/070c3bc8/src
>> /main/org/apache/tools/ant/types/ZipScanner.java
>> ----------------------------------------------------------------------
>> diff --git a/src/main/org/apache/tools/ant/types/ZipScanner.java
>> b/src/main/org/apache/tools/ant/types/ZipScanner.java
>> index a3df040..5667159 100644
>> --- a/src/main/org/apache/tools/ant/types/ZipScanner.java
>> +++ b/src/main/org/apache/tools/ant/types/ZipScanner.java
>> @@ -20,7 +20,7 @@ package org.apache.tools.ant.types;
>>     import java.io.File;
>>   import java.io.IOException;
>> -import java.util.Enumeration;
>> +import java.util.Collections;
>>   import java.util.Map;
>>   import java.util.zip.ZipException;
>>   @@ -62,10 +62,7 @@ public class ZipScanner extends ArchiveScanner {
>>                  "Only file provider resources are supported"));
>>            try (ZipFile zf = new ZipFile(srcFile, encoding)) {
>> -
>> -            Enumeration<ZipEntry> e = zf.getEntries();
>> -            while (e.hasMoreElements()) {
>> -                ZipEntry entry = e.nextElement();
>> +            for (ZipEntry entry : Collections.list(zf.getEntries())) {
>>                  Resource r = new ZipResource(srcFile, encoding, entry);
>>                  String name = entry.getName();
>>                  if (entry.isDirectory()) {
>>
>>
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Jaikiran Pai
To be honest, I don't think this deprecation/conversion change is
good(including this recent commit whichintroduces a
StreamUtils.enumerationAsStream(...))

To give you an example, the code that was previously present would (in
most of the cases) get hold of an enumeration and would iterate over it
and during that iteration would runcertain logic. With the changes that
went in (which again is a bulk change!) the code now gets hold of an
enumeration and instead of just getting on the business of iterating and
running certain logic, instead now passes this enumeration aroundto
convert it into some other form (thus additional code plus additional
objects allocated in the process), all this to iterate over it and run
some logic on it - all of which was already possible with the
enumeration that was already available.


-Jaikiran


On 18/05/18 12:22 AM, Gintautas Grigelionis wrote:

> Thanks for reviewing, I hope Spliterators will do a better job.
>
> Gintas
>
> 2018-05-17 8:37 GMT+02:00 Jaikiran Pai <[hidden email]>:
>
>> I agree. Especially when it's being done on something like for archive
>> entries which can betoo many depending on the archive that is being dealt
>> with.
>>
>> -Jaikiran
>>
>>
>>
>> On 17/05/18 12:04 PM, Maarten Coene wrote:
>>
>>> Converting an Enumeration to a List just for iterating it doesn't seem
>>> performance and memory wise a good idea to me.
>>> Maarten
>>>
>>>         Van: "[hidden email]" <[hidden email]>
>>>    Aan: [hidden email]
>>>    Verzonden: woensdag 16 mei 19:13 2018
>>>    Onderwerp: [1/2] ant git commit: Deprecate CollectionUtils and
>>> Enumerations; reduce explicit use of Enumeration
>>>      Repository: ant
>>> Updated Branches:
>>>     refs/heads/master ac35c0014 -> 070c3bc86
>>>
>>>
>>> http://git-wip-us.apache.org/repos/asf/ant/blob/070c3bc8/src
>>> /main/org/apache/tools/ant/types/ZipScanner.java
>>> ----------------------------------------------------------------------
>>> diff --git a/src/main/org/apache/tools/ant/types/ZipScanner.java
>>> b/src/main/org/apache/tools/ant/types/ZipScanner.java
>>> index a3df040..5667159 100644
>>> --- a/src/main/org/apache/tools/ant/types/ZipScanner.java
>>> +++ b/src/main/org/apache/tools/ant/types/ZipScanner.java
>>> @@ -20,7 +20,7 @@ package org.apache.tools.ant.types;
>>>      import java.io.File;
>>>    import java.io.IOException;
>>> -import java.util.Enumeration;
>>> +import java.util.Collections;
>>>    import java.util.Map;
>>>    import java.util.zip.ZipException;
>>>    @@ -62,10 +62,7 @@ public class ZipScanner extends ArchiveScanner {
>>>                   "Only file provider resources are supported"));
>>>             try (ZipFile zf = new ZipFile(srcFile, encoding)) {
>>> -
>>> -            Enumeration<ZipEntry> e = zf.getEntries();
>>> -            while (e.hasMoreElements()) {
>>> -                ZipEntry entry = e.nextElement();
>>> +            for (ZipEntry entry : Collections.list(zf.getEntries())) {
>>>                   Resource r = new ZipResource(srcFile, encoding, entry);
>>>                   String name = entry.getName();
>>>                   if (entry.isDirectory()) {
>>>
>>>
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Gintautas Grigelionis
I'm not quite sure that what you say was true "in most of the cases".

Gintas

2018-05-18 6:52 GMT+02:00 Jaikiran Pai <[hidden email]>:

> To be honest, I don't think this deprecation/conversion change is
> good(including this recent commit whichintroduces a
> StreamUtils.enumerationAsStream(...))
>
> To give you an example, the code that was previously present would (in
> most of the cases) get hold of an enumeration and would iterate over it and
> during that iteration would runcertain logic. With the changes that went in
> (which again is a bulk change!) the code now gets hold of an enumeration
> and instead of just getting on the business of iterating and running
> certain logic, instead now passes this enumeration aroundto convert it into
> some other form (thus additional code plus additional objects allocated in
> the process), all this to iterate over it and run some logic on it - all of
> which was already possible with the enumeration that was already available.
>
>
> -Jaikiran
>
>
>
> On 18/05/18 12:22 AM, Gintautas Grigelionis wrote:
>
>> Thanks for reviewing, I hope Spliterators will do a better job.
>>
>> Gintas
>>
>> 2018-05-17 8:37 GMT+02:00 Jaikiran Pai <[hidden email]>:
>>
>> I agree. Especially when it's being done on something like for archive
>>> entries which can betoo many depending on the archive that is being dealt
>>> with.
>>>
>>> -Jaikiran
>>>
>>>
>>>
>>> On 17/05/18 12:04 PM, Maarten Coene wrote:
>>>
>>> Converting an Enumeration to a List just for iterating it doesn't seem
>>>> performance and memory wise a good idea to me.
>>>> Maarten
>>>>
>>>>         Van: "[hidden email]" <[hidden email]>
>>>>    Aan: [hidden email]
>>>>    Verzonden: woensdag 16 mei 19:13 2018
>>>>    Onderwerp: [1/2] ant git commit: Deprecate CollectionUtils and
>>>> Enumerations; reduce explicit use of Enumeration
>>>>      Repository: ant
>>>> Updated Branches:
>>>>     refs/heads/master ac35c0014 -> 070c3bc86
>>>>
>>>>
>>>> http://git-wip-us.apache.org/repos/asf/ant/blob/070c3bc8/src
>>>> /main/org/apache/tools/ant/types/ZipScanner.java
>>>> ----------------------------------------------------------------------
>>>> diff --git a/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>> b/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>> index a3df040..5667159 100644
>>>> --- a/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>> +++ b/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>> @@ -20,7 +20,7 @@ package org.apache.tools.ant.types;
>>>>      import java.io.File;
>>>>    import java.io.IOException;
>>>> -import java.util.Enumeration;
>>>> +import java.util.Collections;
>>>>    import java.util.Map;
>>>>    import java.util.zip.ZipException;
>>>>    @@ -62,10 +62,7 @@ public class ZipScanner extends ArchiveScanner {
>>>>                   "Only file provider resources are supported"));
>>>>             try (ZipFile zf = new ZipFile(srcFile, encoding)) {
>>>> -
>>>> -            Enumeration<ZipEntry> e = zf.getEntries();
>>>> -            while (e.hasMoreElements()) {
>>>> -                ZipEntry entry = e.nextElement();
>>>> +            for (ZipEntry entry : Collections.list(zf.getEntries())) {
>>>>                   Resource r = new ZipResource(srcFile, encoding,
>>>> entry);
>>>>                   String name = entry.getName();
>>>>                   if (entry.isDirectory()) {
>>>>
>>>>
>>>>
>>>>
>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email]
>>> For additional commands, e-mail: [hidden email]
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Jaikiran Pai
If your objection is that I claimed that these qualify as "most of the
cases" - I really don't know what else to say then. The original commit
which did this change is this[1]. I haven't reviewed it fully, but the
very first few changes that are done are these [2] [3] [4] [5][6].

Of course, there's a subsequent commit which then uses a different new
util, instead of just using the existing iterator/enumeration. Speaking
of the subsequent commit, it still doesn't undo the (IMO unnecessary)
change that was done to some of the code (take a look at
ArgumentProcessorRegistry.java for example).

Even if these don't fall under "most of the cases", why even change
these places? I'm sure you would know this - the Enumeration or APIs
that you refactored aren't even deprecated in Java version 10.

Anyway, I'm really getting tired of these back and forth arguments on
refactoring. The reason I get involved in certain open source projects
is to get a chance to work with like minded developers and learn
something out of it and not to go wage a war on which coding style is
better or try and be critical of other committers' commits.
Unfortunately, in the recent past, this has reached a state where I have
ended up spending more time being critical of changes that have gone in,
than actually adding much code of value. As much as I try to stay away
from reviews or checking the commit logs, I just keep going back to
them. I don't want to end up being a grumpy guy criticizing the commits.
I'm just going to take a break from this for some days and be a regular
user and come back and see if I still enjoy contributing.

[1]
https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624fe50ae82f0d11171b2

[2]
https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624fe50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL746
[3]
https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624fe50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL834
[4]
https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624fe50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL888
[5]
https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624fe50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL1359

[6]
https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624fe50ae82f0d11171b2#diff-b98a3d2097d6a9b5d7e0fc2eac033f24L348


-Jaikiran


On 18/05/18 11:15 AM, Gintautas Grigelionis wrote:

> I'm not quite sure that what you say was true "in most of the cases".
>
> Gintas
>
> 2018-05-18 6:52 GMT+02:00 Jaikiran Pai <[hidden email]>:
>
>> To be honest, I don't think this deprecation/conversion change is
>> good(including this recent commit whichintroduces a
>> StreamUtils.enumerationAsStream(...))
>>
>> To give you an example, the code that was previously present would (in
>> most of the cases) get hold of an enumeration and would iterate over it and
>> during that iteration would runcertain logic. With the changes that went in
>> (which again is a bulk change!) the code now gets hold of an enumeration
>> and instead of just getting on the business of iterating and running
>> certain logic, instead now passes this enumeration aroundto convert it into
>> some other form (thus additional code plus additional objects allocated in
>> the process), all this to iterate over it and run some logic on it - all of
>> which was already possible with the enumeration that was already available.
>>
>>
>> -Jaikiran
>>
>>
>>
>> On 18/05/18 12:22 AM, Gintautas Grigelionis wrote:
>>
>>> Thanks for reviewing, I hope Spliterators will do a better job.
>>>
>>> Gintas
>>>
>>> 2018-05-17 8:37 GMT+02:00 Jaikiran Pai <[hidden email]>:
>>>
>>> I agree. Especially when it's being done on something like for archive
>>>> entries which can betoo many depending on the archive that is being dealt
>>>> with.
>>>>
>>>> -Jaikiran
>>>>
>>>>
>>>>
>>>> On 17/05/18 12:04 PM, Maarten Coene wrote:
>>>>
>>>> Converting an Enumeration to a List just for iterating it doesn't seem
>>>>> performance and memory wise a good idea to me.
>>>>> Maarten
>>>>>
>>>>>          Van: "[hidden email]" <[hidden email]>
>>>>>     Aan: [hidden email]
>>>>>     Verzonden: woensdag 16 mei 19:13 2018
>>>>>     Onderwerp: [1/2] ant git commit: Deprecate CollectionUtils and
>>>>> Enumerations; reduce explicit use of Enumeration
>>>>>       Repository: ant
>>>>> Updated Branches:
>>>>>      refs/heads/master ac35c0014 -> 070c3bc86
>>>>>
>>>>>
>>>>> http://git-wip-us.apache.org/repos/asf/ant/blob/070c3bc8/src
>>>>> /main/org/apache/tools/ant/types/ZipScanner.java
>>>>> ----------------------------------------------------------------------
>>>>> diff --git a/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>> b/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>> index a3df040..5667159 100644
>>>>> --- a/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>> +++ b/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>> @@ -20,7 +20,7 @@ package org.apache.tools.ant.types;
>>>>>       import java.io.File;
>>>>>     import java.io.IOException;
>>>>> -import java.util.Enumeration;
>>>>> +import java.util.Collections;
>>>>>     import java.util.Map;
>>>>>     import java.util.zip.ZipException;
>>>>>     @@ -62,10 +62,7 @@ public class ZipScanner extends ArchiveScanner {
>>>>>                    "Only file provider resources are supported"));
>>>>>              try (ZipFile zf = new ZipFile(srcFile, encoding)) {
>>>>> -
>>>>> -            Enumeration<ZipEntry> e = zf.getEntries();
>>>>> -            while (e.hasMoreElements()) {
>>>>> -                ZipEntry entry = e.nextElement();
>>>>> +            for (ZipEntry entry : Collections.list(zf.getEntries())) {
>>>>>                    Resource r = new ZipResource(srcFile, encoding,
>>>>> entry);
>>>>>                    String name = entry.getName();
>>>>>                    if (entry.isDirectory()) {
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: [hidden email]
>>>> For additional commands, e-mail: [hidden email]
>>>>
>>>>
>>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Gintautas Grigelionis
I accepted the original criticism that going Enumeration -> List -> Stream
has an overhead and I tried to address that by using a decorator.
I believe that Streams API can at least implement the logic run by an
original Enumeration in a more concise way, or provide more powerful idioms.
That IMO makes it worth the while to investigate the Streams alternatives.

In the process I also found out that other iterators could be used in a
more efficient way, or that there is a redundant object construction going
on.
If you wish, I may spend some time to describe the changes I introduced,
and maybe then we could discuss the their merits.

Gintas

2018-05-18 8:30 GMT+02:00 Jaikiran Pai <[hidden email]>:

> If your objection is that I claimed that these qualify as "most of the
> cases" - I really don't know what else to say then. The original commit
> which did this change is this[1]. I haven't reviewed it fully, but the very
> first few changes that are done are these [2] [3] [4] [5][6].
>
> Of course, there's a subsequent commit which then uses a different new
> util, instead of just using the existing iterator/enumeration. Speaking of
> the subsequent commit, it still doesn't undo the (IMO unnecessary) change
> that was done to some of the code (take a look at
> ArgumentProcessorRegistry.java for example).
>
> Even if these don't fall under "most of the cases", why even change these
> places? I'm sure you would know this - the Enumeration or APIs that you
> refactored aren't even deprecated in Java version 10.
>
> Anyway, I'm really getting tired of these back and forth arguments on
> refactoring. The reason I get involved in certain open source projects is
> to get a chance to work with like minded developers and learn something out
> of it and not to go wage a war on which coding style is better or try and
> be critical of other committers' commits. Unfortunately, in the recent
> past, this has reached a state where I have ended up spending more time
> being critical of changes that have gone in, than actually adding much code
> of value. As much as I try to stay away from reviews or checking the commit
> logs, I just keep going back to them. I don't want to end up being a grumpy
> guy criticizing the commits. I'm just going to take a break from this for
> some days and be a regular user and come back and see if I still enjoy
> contributing.
>
> [1] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2
>
> [2] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL746
> [3] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL834
> [4] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL888
> [5] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL1359
>
> [6] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2#diff-b98a3d2097d6a9b5d7e0fc2eac033f24L348
>
>
> -Jaikiran
>
>
>
> On 18/05/18 11:15 AM, Gintautas Grigelionis wrote:
>
>> I'm not quite sure that what you say was true "in most of the cases".
>>
>> Gintas
>>
>> 2018-05-18 6:52 GMT+02:00 Jaikiran Pai <[hidden email]>:
>>
>> To be honest, I don't think this deprecation/conversion change is
>>> good(including this recent commit whichintroduces a
>>> StreamUtils.enumerationAsStream(...))
>>>
>>> To give you an example, the code that was previously present would (in
>>> most of the cases) get hold of an enumeration and would iterate over it
>>> and
>>> during that iteration would runcertain logic. With the changes that went
>>> in
>>> (which again is a bulk change!) the code now gets hold of an enumeration
>>> and instead of just getting on the business of iterating and running
>>> certain logic, instead now passes this enumeration aroundto convert it
>>> into
>>> some other form (thus additional code plus additional objects allocated
>>> in
>>> the process), all this to iterate over it and run some logic on it - all
>>> of
>>> which was already possible with the enumeration that was already
>>> available.
>>>
>>>
>>> -Jaikiran
>>>
>>>
>>>
>>> On 18/05/18 12:22 AM, Gintautas Grigelionis wrote:
>>>
>>> Thanks for reviewing, I hope Spliterators will do a better job.
>>>>
>>>> Gintas
>>>>
>>>> 2018-05-17 8:37 GMT+02:00 Jaikiran Pai <[hidden email]>:
>>>>
>>>> I agree. Especially when it's being done on something like for archive
>>>>
>>>>> entries which can betoo many depending on the archive that is being
>>>>> dealt
>>>>> with.
>>>>>
>>>>> -Jaikiran
>>>>>
>>>>>
>>>>>
>>>>> On 17/05/18 12:04 PM, Maarten Coene wrote:
>>>>>
>>>>> Converting an Enumeration to a List just for iterating it doesn't seem
>>>>>
>>>>>> performance and memory wise a good idea to me.
>>>>>> Maarten
>>>>>>
>>>>>>          Van: "[hidden email]" <[hidden email]>
>>>>>>     Aan: [hidden email]
>>>>>>     Verzonden: woensdag 16 mei 19:13 2018
>>>>>>     Onderwerp: [1/2] ant git commit: Deprecate CollectionUtils and
>>>>>> Enumerations; reduce explicit use of Enumeration
>>>>>>       Repository: ant
>>>>>> Updated Branches:
>>>>>>      refs/heads/master ac35c0014 -> 070c3bc86
>>>>>>
>>>>>>
>>>>>> http://git-wip-us.apache.org/repos/asf/ant/blob/070c3bc8/src
>>>>>> /main/org/apache/tools/ant/types/ZipScanner.java
>>>>>> ------------------------------------------------------------
>>>>>> ----------
>>>>>> diff --git a/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>>> b/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>>> index a3df040..5667159 100644
>>>>>> --- a/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>>> +++ b/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>>> @@ -20,7 +20,7 @@ package org.apache.tools.ant.types;
>>>>>>       import java.io.File;
>>>>>>     import java.io.IOException;
>>>>>> -import java.util.Enumeration;
>>>>>> +import java.util.Collections;
>>>>>>     import java.util.Map;
>>>>>>     import java.util.zip.ZipException;
>>>>>>     @@ -62,10 +62,7 @@ public class ZipScanner extends ArchiveScanner
>>>>>> {
>>>>>>                    "Only file provider resources are supported"));
>>>>>>              try (ZipFile zf = new ZipFile(srcFile, encoding)) {
>>>>>> -
>>>>>> -            Enumeration<ZipEntry> e = zf.getEntries();
>>>>>> -            while (e.hasMoreElements()) {
>>>>>> -                ZipEntry entry = e.nextElement();
>>>>>> +            for (ZipEntry entry : Collections.list(zf.getEntries()))
>>>>>> {
>>>>>>                    Resource r = new ZipResource(srcFile, encoding,
>>>>>> entry);
>>>>>>                    String name = entry.getName();
>>>>>>                    if (entry.isDirectory()) {
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: [hidden email]
>>>>> For additional commands, e-mail: [hidden email]
>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email]
>>> For additional commands, e-mail: [hidden email]
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Johan Corveleyn
On Fri, May 18, 2018 at 10:07 AM, Gintautas Grigelionis
<[hidden email]> wrote:

> I accepted the original criticism that going Enumeration -> List -> Stream
> has an overhead and I tried to address that by using a decorator.
> I believe that Streams API can at least implement the logic run by an
> original Enumeration in a more concise way, or provide more powerful idioms.
> That IMO makes it worth the while to investigate the Streams alternatives.
>
> In the process I also found out that other iterators could be used in a
> more efficient way, or that there is a redundant object construction going
> on.
> If you wish, I may spend some time to describe the changes I introduced,
> and maybe then we could discuss the their merits.
>
> Gintas

As a lurker on this list, but also Java developer in $dayjob [1], I'd
like to add my 2 cents, FWIW.

I see two things going on that are causing a lot of irritation / frustration:

1) Large codebase-wide changes for adapting old (well working) code to
new API's, syntax, idioms, ... usually mostly cosmetic. From my
personal perspective (I know, I carry little weight here, I'm just an
innocent bystander) this is not a good idea. Except in specific cases
for example for security reasons, or where there is a clear benefit
also for users (for example it improves performance significantly in
certain scenarios -- Note that I say "significantly", because shaving
off 1 byte off an operation that's only run once by normal users is
not worth the code churn / review effort / risk (yes that means you
need to measure and show the significant improvement if you want to
use this as an argument)).

I remember years ago we had a similar discussion at $dayjob, when
someone proposed to standardize our rules for grouping import
statements. "Should we now bulk-change our entire codebase?" After
discussion the consensus was "no" (needless code churn, review burden,
possible future backport conflicts, ...), except when you're going to
edit the class anyway. So the simple rule was: "use the new style in
new code, and in code that you're going to change for 'external
reasons' anyway".


Which brings me to the more important problem:

2) There is clearly no consensus here on the merits / desirability of
these bulk changes. Gintas, I understand your desire to move things
along, and you clearly have an opinion about it, but IMHO you really
need to stop doing any more bulk changes as long as there is no
consensus.

Several people have voiced objections on this lists ([2], [3], [4],
...), so why are you continuing to do this? When people start raising
flags you really should pause for a moment, and start discussing
before making more commits. By making further disputable commits you
are making the discussion more difficult (it's much more difficult to
freely speak your mind if the code has already been changed ... the
work has already been done and it's even more work to revert it, ...).


[1] Full disclosure: I work at the same company as Maarten Coene. I'm
also an ASF member and PMC member of the Subversion project, but that
doesn't really matter here.

[2] https://mail-archives.apache.org/mod_mbox/ant-dev/201804.mbox/%3C5682399f-c52e-49bd-a48b-740feaf60653%40apache.org%3E
and its entire thread
[3] https://mail-archives.apache.org/mod_mbox/ant-dev/201804.mbox/%3C001301d3e070%246555ff10%243001fd30%24%40de%3E
and previous answers by other committers in that same thread.
[4] https://mail-archives.apache.org/mod_mbox/ant-dev/201805.mbox/%3C87efitnm5f.fsf_-_%40v45346.1blu.de%3E

--
Johan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Stefan Bodewig
In reply to this post by Gintautas Grigelionis
On 2018-05-18, Gintautas Grigelionis wrote:

> I believe that Streams API can at least implement the logic run by an
> original Enumeration in a more concise way, or provide more powerful idioms.
> That IMO makes it worth the while to investigate the Streams alternatives.

I agree to do that as soon as we want to change the code to do something
that wants to use said idioms. I don't really see a reason to change the
code before that point in time.

> In the process I also found out that other iterators could be used in a
> more efficient way, or that there is a redundant object construction going
> on.
> If you wish, I may spend some time to describe the changes I introduced,
> and maybe then we could discuss the their merits.

Let's use the concrete example that raised Maarten's email. In
ZipScanner we've gone from

            Enumeration<ZipEntry> e = zf.getEntries();
            while (e.hasMoreElements()) {
                ZipEntry entry = e.nextElement();

to

            StreamUtils.enumerationAsStream(zf.getEntries()).forEach(entry -> {

the Enumeration instance is created in both cases. What is the benfit
for the ZipScanner class right now?

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Gintautas Grigelionis
2018-05-18 16:51 GMT+02:00 Stefan Bodewig <[hidden email]>:

> On 2018-05-18, Gintautas Grigelionis wrote:
>
> > I believe that Streams API can at least implement the logic run by an
> > original Enumeration in a more concise way, or provide more powerful
> idioms.
> > That IMO makes it worth the while to investigate the Streams
> alternatives.
>
> I agree to do that as soon as we want to change the code to do something
> that wants to use said idioms. I don't really see a reason to change the
> code before that point in time.
>
> > In the process I also found out that other iterators could be used in a
> > more efficient way, or that there is a redundant object construction
> going
> > on.
> > If you wish, I may spend some time to describe the changes I introduced,
> > and maybe then we could discuss the their merits.
>
> Let's use the concrete example that raised Maarten's email. In
> ZipScanner we've gone from
>
>             Enumeration<ZipEntry> e = zf.getEntries();
>             while (e.hasMoreElements()) {
>                 ZipEntry entry = e.nextElement();
>
> to
>
>             StreamUtils.enumerationAsStream(zf.getEntries()).forEach(entry
> -> {
>
> the Enumeration instance is created in both cases. What is the benfit
> for the ZipScanner class right now?
>

FWIW, the original java.util.ZipFile has a stream() that is based on an
iterator since Java 8 [1].
What is the reason for adding that method if Enumeration is so much better?
;-)
I dare say, the benefit is that streams are parallelizable.

Gintas

[1]
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/zip/ZipFile.java#ZipFile.stream%28%29
Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Stefan Bodewig
On 2018-05-20, Gintautas Grigelionis wrote:

> 2018-05-18 16:51 GMT+02:00 Stefan Bodewig <[hidden email]>:

>> On 2018-05-18, Gintautas Grigelionis wrote:

>>> I believe that Streams API can at least implement the logic run by an
>>> original Enumeration in a more concise way, or provide more powerful
>> idioms.
>>> That IMO makes it worth the while to investigate the Streams
>> alternatives.

>> I agree to do that as soon as we want to change the code to do something
>> that wants to use said idioms. I don't really see a reason to change the
>> code before that point in time.

>>> In the process I also found out that other iterators could be used in a
>>> more efficient way, or that there is a redundant object construction
>> going
>>> on.
>>> If you wish, I may spend some time to describe the changes I introduced,
>>> and maybe then we could discuss the their merits.

>> Let's use the concrete example that raised Maarten's email. In
>> ZipScanner we've gone from

>>             Enumeration<ZipEntry> e = zf.getEntries();
>>             while (e.hasMoreElements()) {
>>                 ZipEntry entry = e.nextElement();

>> to

>>             StreamUtils.enumerationAsStream(zf.getEntries()).forEach(entry
>> -> {

>> the Enumeration instance is created in both cases. What is the benfit
>> for the ZipScanner class right now?


> FWIW, the original java.util.ZipFile has a stream() that is based on an
> iterator since Java 8 [1].
> What is the reason for adding that method if Enumeration is so much better?
> ;-)
> I dare say, the benefit is that streams are parallelizable.

I'm afraid you misunderstood my question. I didn't ask why streams may
be preferable over Enumerations when you write new code or change
existing code. I was asking how does this benefit the existing code
right now when there is no other change at all.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Gintautas Grigelionis
In reply to this post by Johan Corveleyn
2018-05-18 16:11 GMT+02:00 Johan Corveleyn <[hidden email]>:

> 2) There is clearly no consensus here on the merits / desirability of
> these bulk changes. Gintas, I understand your desire to move things
> along, and you clearly have an opinion about it, but IMHO you really
> need to stop doing any more bulk changes as long as there is no
> consensus.
>

This time, I tried to start a discussion first, and I got no response.
There were attempts to modernize the codebase about a year ago. AFAICS
there was no discussion then.
What was that saying: "don't vote, don't complain"? I'd like some
discussion, that's all.

I hear that the consensus is "new stuff is for new code, or at least
whenever one happens to change old code fixing the bugs".
I'm not quite sure whether that's a good idea, we should at least try to
achieve some consistency
-- and not necessarily sticking to the bottom line.
Or maybe I'm too old school seeing major version changes as necessitating a
porting effort ;-)

Gintas
Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Gintautas Grigelionis
In reply to this post by Stefan Bodewig
2018-05-20 16:08 GMT+02:00 Stefan Bodewig <[hidden email]>:

> I'm afraid you misunderstood my question. I didn't ask why streams may
> be preferable over Enumerations when you write new code or change
> existing code. I was asking how does this benefit the existing code
> right now when there is no other change at all.
>

Sorry if I was unclear but the point was that the stream is parallelizable.
So, potentially higher thoughput on multicore?

Gintas
Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Gintautas Grigelionis
In reply to this post by Gintautas Grigelionis
2018-05-20 21:05 GMT+02:00 Gintautas Grigelionis <[hidden email]>:

> 2018-05-18 16:11 GMT+02:00 Johan Corveleyn <[hidden email]>:
>
>> 2) There is clearly no consensus here on the merits / desirability of
>> these bulk changes. Gintas, I understand your desire to move things
>> along, and you clearly have an opinion about it, but IMHO you really
>> need to stop doing any more bulk changes as long as there is no
>> consensus.
>>
>
> This time, I tried to start a discussion first, and I got no response.
> There were attempts to modernize the codebase about a year ago. AFAICS
> there was no discussion then.
> What was that saying: "don't vote, don't complain"? I'd like some
> discussion, that's all.
>
> I hear that the consensus is "new stuff is for new code, or at least
> whenever one happens to change old code fixing the bugs".
> I'm not quite sure whether that's a good idea, we should at least try to
> achieve some consistency
> -- and not necessarily sticking to the bottom line.
> Or maybe I'm too old school seeing major version changes as necessitating
> a porting effort ;-)
>

Just another .02€ -- if we agree on porting guidelines, there's no need to
squabble about "personal preferences".
At the moment, the only "global" issues I see are "informally deprecated"
JRE APIs (Vector, Hashtable, Stack,...) and the use of streams/lambdas.

Gintas
Reply | Threaded
Open this post in threaded view
|

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration

Stefan Bodewig
In reply to this post by Gintautas Grigelionis
On 2018-05-20, Gintautas Grigelionis wrote:

> This time, I tried to start a discussion first, and I got no response.
> There were attempts to modernize the codebase about a year ago. AFAICS
> there was no discussion then.

True. I for one didn't expect you to try weeding out all deprecated APIs
and TBH didn't understand what you've been aking for.

> I hear that the consensus is "new stuff is for new code, or at least
> whenever one happens to change old code fixing the bugs".

Yes, please, this would be my preference.

What we have seen now are:

* changes that don't get proper review because they are too big

* mistakes (that everybody makes, I'm not complaining about them) slip
  through because of the point above

* willing contributors get frustrated and say they'll stop reviewing
  anthing or even drop out of the project

* merging from the 1.9 branch to master gets more difficult

For me these drawbacks outweigh all benefits of consistency.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]