Use JPSG instead of gen.py in eclipse-collections; extract some common functionality into primitive-collections-base module #51

leventov · 2019-05-22T15:16:14Z

This PR is half of the work that should be done towards #38.

It extracts common functionality from eclipse-collections module into primitive-collections-base.

It also replaces the use of eclipse-collections' gen.py with JPSG which allows removing huge amount of repetitive code across primitive collection combinations (especially maps).

…n functionality into primitive-collections-base module

cowtowncoder · 2019-05-22T20:13:51Z

Ok, so I think this makes sense at high level. Just couple of questions:

Would it make sense to separate JPSG change from others -- also I'd like @yawkat 's input on this change
What would make most sense wrt 2.10 vs 3.0: adding base classes makes sense for 3.0 (master), but I wonder if there's any intermediate step that would make sense for 2.10 -- maybe base classes could be for new fastutil module, and 3.0 would then refactor existing ones (include eclipse-collections) to use it?

yawkat · 2019-05-22T20:40:56Z

I agree with this change in principle, the python script was a hack, this is much better. The code looks good too. My only concern is compatibility, I'm not entirely sure the test suite I wrote covers everything, but these changes look solid enough

yawkat · 2019-05-22T20:42:27Z

Oh also, the package name is a bit odd (underscores) and against the google style guide, but I don't know how jackson handles that (it doesn't bother me too much)

cowtowncoder · 2019-05-22T22:08:10Z

Ok so if JPSG change could be applied against 2.10, that'd be great I think. And maybe separate it into different PR. Would be happy to merge (I assume we have a CLA from you @leventov ? If not -- I can verify later when I have access to filed ones -- template is here https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf )

leventov · 2019-05-23T15:55:52Z

What would make most sense wrt 2.10 vs 3.0: adding base classes makes sense for 3.0 (master), but I wonder if there's any intermediate step that would make sense for 2.10 -- maybe base classes could be for new fastutil module, and 3.0 would then refactor existing ones (include eclipse-collections) to use it?

Am I understand right that you propose to bring fastutil to 2.10 by just adding the new module and the base module without basically touching the eclipse-collections module, while restructuring eclipse-collections module only in master? Then, this PR (to master) shouldn't be changed much?

I can create a different PR to 2.10, but separating JPSG change would be hard because unfortunately, I was doing the changes (JPSG and refactoring into a basic module) in parallel and they are not quite a clear cut, so if possible I would like to avoid doing that.

I've sent CLA to [email protected].

cowtowncoder · 2019-05-27T00:33:04Z

Correct, I was thinking of adding with minimal changes in 2.10.

However. If and when changes are only for Eclipse-collections and not for earlier added collections (...until master), actually that is fine: EC is officially only added in 2.10, so I think even major refactoring is fine.

So getting it all in for 2.10 just for EC/Fastutil is fine.

leventov · 2019-05-27T18:09:50Z

Is there something more that has to be done in this PR?

cowtowncoder · 2019-05-29T02:46:28Z

.../java/com/fasterxml/jackson/datatype/eclipsecollections/EclipseCollectionsDeserializers.java

@@ -156,9 +155,11 @@
    // are faster to construct.

    // initialized below
-    static final Map<Class<? extends PrimitiveIterable>, JsonDeserializer<?>> PRIMITIVE_DESERIALIZERS = new HashMap<>();
+    static final Map<Class<? extends PrimitiveIterable>, JsonDeserializer<?>> PRIMITIVE_DESERIALIZERS
+            = new IdentityHashMap<>();


Is there a specific reason to use IdentityHashMap here?

IdentityHashMap should be a little faster in this case, which may not be a totally pointless optimization because getting serializers/deserializers by class is an operation on the hot path. I use IdentityHashMap in new code (PrimitivePrimitiveMapSerializers - in this PR, and in the (yet unchecked) fastutil module which should come in a follow-up PR), so I aligned existing code to use IdentityHashMap for consistency, too.

It'd be good to measure this. I do remember Class<?> lookup being on hot path because it does not override default equals() or hashCode(), but I also recall IdentityHashMap itself not being super efficient. What core databind does is actually uses ClassKey type, but while I think that removed a hot spot I don't think I ever properly verified its effect with JMH.

Having said all of that, I do not think this lookup should be on critical path as DeserializerCache used by jackson-databind should handle caching by type and this provider should not be called more than once.

Ok, reverted to use HashMap

cowtowncoder · 2019-05-29T02:46:56Z

.../java/com/fasterxml/jackson/datatype/eclipsecollections/EclipseCollectionsDeserializers.java

    @SuppressWarnings("rawtypes")
-    static final Set<Class<? extends InternalIterable>> REFERENCE_TYPES = new HashSet<>();
+    static final Set<Class<? extends InternalIterable>> REFERENCE_TYPES =
+            Collections.newSetFromMap(new IdentityHashMap<>());


Similarly here... what is the point of change?

cowtowncoder · 2019-05-29T02:48:15Z

Added one question (well 2 but basically the same). But aside from that, if the hope is to get this in 2.10, it'd need to be based from 2.10. Would it be possible to rebase changes to start from 2.10 branch instead of master?

…ns-base

leventov · 2019-10-16T16:18:56Z

Although this PR now passes CI, it must not be merged quite yet, because I suspect that module-info.java is not correct. #61 for details.

…llections' module-info.java

leventov · 2019-10-17T12:04:55Z

@cowtowncoder I've retained requires transitive in primitive-collections-base's module-info.java because it seems reasonable. Also added an entry to eclipse-collections' module-info.java.

I believe now this PR should be ready for merge.

cowtowncoder · 2019-10-20T03:03:12Z

Ok I hope to have another look, will add to my TODO list.
@yawkat Any other last comments?

yawkat · 2019-10-20T10:09:29Z

eclipse-collections/pom.xml

+                <artifactId>maven-surefire-plugin</artifactId>
+                <configuration>
+                    <excludes>
+                        <exclude>com/fasterxml/jackson/**/failing/*.java</exclude>


Is this still required? I assume this was for #54

I copied this from jackson-datatype-collections/pom.xml. It is a general config, apparently, for local development, where failing tests could be temporarily moved to ...failing package. Not related to #54.

However, this config is inherited from jackson-datatype-collections/pom.xml in eclipse-collections.xml anyway, so I removed it here.

I've also found that guava, pcollections, and primitive-collections-base lacked the spec of maven-surefire-plugin and the tests were not run during install goal. Added maven-surefire-plugin to the config of these modules.

yawkat · 2019-10-20T10:12:44Z

I have two comments.

Not sure if this came up, but is the package name primitive_collections_base okay? I know some but not all code styles would forbid it.
The generated artifacts look odd to me. First, the binary artifacts of both the eclipse-collections and the primitive-collections-base modules seem to contain the classes in the primitive_collections_base package. Second, the source artifacts (as generated by source:jar) seem to contain the generated code in the jpsg top-level package which does not match the location in the binary jar. Not sure if this is intentional.

…ve-collections-base; remove unnecessary surefire plugin config in eclipse-collections

…collections-base/pom.xml to align with the rest of the project

leventov · 2019-10-20T10:50:33Z

@yawkat

Second, the source artifacts (as generated by source:jar) seem to contain the generated code in the jpsg top-level package which does not match the location in the binary jar. Not sure if this is intentional.

It seems to be the way Maven's source code generation convention works. I don't see what I could change in this code: https://github.com/TimeAndSpaceIO/jpsg-maven-plugin/blob/master/src/main/java/io/timeandspace/jpsg/GeneratorMojo.java to fix this, even if this is considered a problem. Presumably, Antlr and other code generators work the same way. Theoretically, it's possible to specify generic generated-sources/ output directory in plugin configuration instead of generated-sources/jpsg/, but that's not the best practice because it would make unclear from where certain generated sources originate.

cowtowncoder · 2019-10-20T15:53:23Z

guava/pom.xml

@@ -57,6 +57,10 @@ com.google.common.*;version="${version.guava.osgi}",
        <groupId>org.moditect</groupId>
        <artifactId>moditect-maven-plugin</artifactId>
      </plugin>
+      <plugin>


Not sure why this would be needed: parent pom (jackson-datatypes-collections) has it with exclusion for failing (which is to allow running failing-for-now tests from IDE without blocking build).

The mvn clean install output with and without this addition is different. Apparently, Guava's tests are not run without it. The spec in jackson-datatypes-collections is in <pluginManagement> element, while here it's in <build><plugins>.

That is odd: for me mvn clean install from main level does run all the tests on master.
Note, too, that none of child projects has separate surefire run settings and tests are run.

You are right. maven-surefire-plugin config in <build><plugins> is actually inherited from Jackson's oss-parent pom. This could be verified by generating Effective Poms. And now, I cannot reproduce the tests not running without these configs (they run now for me), maybe it was my mental glitch.

Ok. Stuff happens, I just wanted to make sure I understood reasoning -- I would not want tests to be accidentally skipped for sure.

I hope to merge this soon, today or tomorrow. Thank you for all the work here!

cowtowncoder · 2019-10-20T15:53:46Z

pcollections/pom.xml

@@ -46,6 +46,10 @@ PCollections (http://pcollections.org/) types
        <groupId>org.moditect</groupId>
        <artifactId>moditect-maven-plugin</artifactId>
      </plugin>
+      <plugin>


Ditto here, parent has it.

cowtowncoder · 2019-10-20T15:55:07Z

One more question: would hppc depend on the new primitive-base jar in future? Or just the fastutils one.

leventov · 2019-10-20T18:24:04Z

One more question: would hppc depend on the new primitive-base jar in future? Or just the fastutils one.

Personally, I plan just to add a module for fastutils as the next step.

…ion to parent's <pluginManagement>

Use JPSG instead of gen.py in eclipse-collections; extract some commo…

cf27e3e

…n functionality into primitive-collections-base module

cowtowncoder reviewed May 29, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into primitive-collectio…

1d2313b

…ns-base

leventov mentioned this pull request Oct 14, 2019

Document how to generate module files using moditect #61

Closed

Add module-infos, update jpsg dependency

3c15d2e

Use HashMap instead of IdentityHashMap; warning fixes; fix eclipse-co…

7ad2141

…llections' module-info.java

yawkat approved these changes Oct 20, 2019

View reviewed changes

yawkat reviewed Oct 20, 2019

View reviewed changes

leventov added 2 commits October 20, 2019 13:30

Add missing surefire plugin specs in guava, pcollections, and primiti…

a3f3328

…ve-collections-base; remove unnecessary surefire plugin config in eclipse-collections

Use 2-space indentation in eclipse-collections/pom.xml and primitive-…

bceaf80

…collections-base/pom.xml to align with the rest of the project

cowtowncoder reviewed Oct 20, 2019

View reviewed changes

Remove unnecessary surefire plugin declarations; move jpsg configurat…

e0e6bc8

…ion to parent's <pluginManagement>

cowtowncoder merged commit 66f7307 into FasterXML:master Oct 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use JPSG instead of gen.py in eclipse-collections; extract some common functionality into primitive-collections-base module #51

Use JPSG instead of gen.py in eclipse-collections; extract some common functionality into primitive-collections-base module #51

leventov commented May 22, 2019

cowtowncoder commented May 22, 2019

yawkat commented May 22, 2019

yawkat commented May 22, 2019

cowtowncoder commented May 22, 2019

leventov commented May 23, 2019

cowtowncoder commented May 27, 2019

leventov commented May 27, 2019

cowtowncoder May 29, 2019

leventov Oct 16, 2019

cowtowncoder Oct 16, 2019

leventov Oct 17, 2019

cowtowncoder May 29, 2019

cowtowncoder commented May 29, 2019

leventov commented Oct 16, 2019

leventov commented Oct 17, 2019

cowtowncoder commented Oct 20, 2019

yawkat Oct 20, 2019

leventov Oct 20, 2019

yawkat commented Oct 20, 2019

leventov commented Oct 20, 2019

cowtowncoder Oct 20, 2019

leventov Oct 20, 2019

cowtowncoder Oct 21, 2019

leventov Oct 21, 2019

cowtowncoder Oct 21, 2019

cowtowncoder Oct 20, 2019

cowtowncoder commented Oct 20, 2019 •

edited

Loading

leventov commented Oct 20, 2019

Use JPSG instead of gen.py in eclipse-collections; extract some common functionality into primitive-collections-base module #51

Use JPSG instead of gen.py in eclipse-collections; extract some common functionality into primitive-collections-base module #51

Conversation

leventov commented May 22, 2019

cowtowncoder commented May 22, 2019

yawkat commented May 22, 2019

yawkat commented May 22, 2019

cowtowncoder commented May 22, 2019

leventov commented May 23, 2019

cowtowncoder commented May 27, 2019

leventov commented May 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented May 29, 2019

leventov commented Oct 16, 2019

leventov commented Oct 17, 2019

cowtowncoder commented Oct 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yawkat commented Oct 20, 2019

leventov commented Oct 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Oct 20, 2019 • edited Loading

leventov commented Oct 20, 2019

cowtowncoder commented Oct 20, 2019 •

edited

Loading