Load encoded values from disk #2542

easbar · 2022-03-21T12:07:21Z

I tried using Jackson to serialize/deserialize an EnumEncodedValue.
For example a RoadClass EV is serialized to:

{"className":"com.graphhopper.routing.ev.EnumEncodedValue","name":"road_class","bits":5,"minValue":0,"maxValue":31,"negateReverseDirection":false,"storeTwoDirections":false,"enumType":"com.graphhopper.routing.ev.RoadClass"}

For EnumEncodedValue we do not store the enum values explicitly, but rather the name of the enum class. For StringEncodedValue we simply store all the values.

core/src/test/java/com/graphhopper/routing/ev/EnumEncodedValueTest.java

easbar · 2022-03-21T14:08:00Z

core/src/test/java/com/graphhopper/routing/ev/EnumEncodedValueTest.java

+        // serialize
+        List<String> serializedEVs = new ArrayList<>();
+        for (EncodedValue e : encodedValues) {
+            String s = mapper.writeValueAsString(e);
+            serializedEVs.add(s);
+        }
+
+        // deserialize
+        List<EncodedValue> deserializedEVs = new ArrayList<>();
+        for (String s : serializedEVs) {
+            JsonNode jsonNode = mapper.readTree(s);
+            deserializedEVs.add(mapper.treeToValue(jsonNode, EncodedValue.class));
+        }


This is pretty nice and seems to work for all our EncodedValue subclasses automatically, due to the @JsonTypeInfo annotation that I added to the EncodedValue interface.

…er because of subclass constructors

easbar · 2022-03-21T17:03:06Z

core/src/main/java/com/graphhopper/routing/ev/EncodedValueSerializer.java

+            int storedVersion = jsonNode.get("version").asInt();
+            ((ObjectNode) jsonNode).remove("version");
+            EncodedValue encodedValue = MAPPER.treeToValue(jsonNode, EncodedValue.class);
+            if (storedVersion != encodedValue.getVersion())
+                throw new IllegalStateException("Version does not match. Cannot properly read encoded value: " + encodedValue.getName() + ". " +
+                        "You need to use the same version of GraphHopper you used to import the data");


This is a bit self-made and probably can also be done using Jackson Annotations, but since we aren't really planning to de/serialize encoded values with something other than these two methods I think this is fine. I'm not so experienced with Jackson and open for improvements of course.

I'm also no expert. I tried to get it working and embedded your work into a common Deserializer (and using @JsonProperty(access = JsonProperty.Access.READ_ONLY) for getVersion to avoid a custom Serializer).

But the problem was that we don't want to manually handle the className and I wasn't able to find a way to use both:
@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, property = "className")
and
@JsonDeserialize(using = EncodedValueDeserializer.class).

The only solution was with some wrapper class but this here looks simpler IMO.

otbutz · 2022-03-21T16:50:19Z

core/src/main/java/com/graphhopper/routing/ev/EnumEncodedValue.java

@@ -25,14 +28,30 @@
 * This class allows to store distinct values via an enum. I.e. it stores just the indices
 */
 public final class EnumEncodedValue<E extends Enum> extends IntEncodedValueImpl {
-    private final E[] arr;
+    public final Class<E> enumType;


Can this be reverted back to private ?

Ah yes, thanks I forgot.

otbutz · 2022-03-21T17:07:24Z

core/src/main/java/com/graphhopper/routing/ev/StringEncodedValue.java

@@ -30,7 +27,7 @@ public StringEncodedValue(String name, int expectedValueCount, boolean storeTwoD

        this.maxValues = roundUp(expectedValueCount);
        this.values = new ArrayList<>(maxValues);
-        this.indexMap = new ObjectIntHashMap<>(maxValues);
+        this.indexMap = new HashMap<>(maxValues);


Could we avoid this with a dedicated getter instead of using field access? I'm not sure if we want to deal with auto(un)boxing in a potentially hot code path.

Does this really not work because we use field access? I thought we need a custom serializer for hppc, but I might be wrong. I thought about the auto unboxing for a moment but then thought it's probably not as bad bc the maps will be very small. But you are more than welcome to make this work with the hppc map again. I just did not feel like spending a lot more time on this, also because currently the default GraphHopper server does not use StringEncodedValue currently.

I tried changing indexMap to the concrete type ObjectIntHashMap and got another error which is due to the fact that HashOrderMixingStrategy is an abstract type and Jackson does not know which type to use upon de-serialization.

It works when we add jackson-datatype-hppc: https://github.com/FasterXML/jackson-datatypes-collections
I tried this here including a little speed test: fa80be9
I could not measure a notable difference, though so I would go with java.util for now. You are invited to prove the opposite, of course.

easbar · 2022-03-21T18:38:41Z

I went for the all-args constructor approach where we can keep the fields final but have to add some all-args constructors for Jackson.

@otbutz maybe this is related to #2243?

otbutz · 2022-03-22T08:00:59Z

Yes this would also solve #2243 😉

Does Jackson require those constructors to be public? Maybe we could reduce the visibility if they're only intended to be used by Jackson.

easbar · 2022-03-22T08:09:28Z

Does Jackson require those constructors to be public? Maybe we could reduce the visibility if they're only intended to be used by Jackson.

Yes, package private also works and is better thanks. We could also make them protected, but not sure if realistically users want to create their own subclasses of IntEncodedValueImpl etc.

karussell

👍

karussell · 2022-03-22T11:36:29Z

core/src/main/java/com/graphhopper/routing/ev/EncodedValueSerializer.java

+            int storedVersion = jsonNode.get("version").asInt();
+            ((ObjectNode) jsonNode).remove("version");
+            EncodedValue encodedValue = MAPPER.treeToValue(jsonNode, EncodedValue.class);
+            if (storedVersion != encodedValue.getVersion())
+                throw new IllegalStateException("Version does not match. Cannot properly read encoded value: " + encodedValue.getName() + ". " +
+                        "You need to use the same version of GraphHopper you used to import the data");


I'm also no expert. I tried to get it working and embedded your work into a common Deserializer (and using @JsonProperty(access = JsonProperty.Access.READ_ONLY) for getVersion to avoid a custom Serializer).

But the problem was that we don't want to manually handle the className and I wasn't able to find a way to use both:
@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, property = "className")
and
@JsonDeserialize(using = EncodedValueDeserializer.class).

The only solution was with some wrapper class but this here looks simpler IMO.

karussell · 2022-03-22T11:37:08Z

core/src/main/java/com/graphhopper/routing/ev/EncodedValueSerializer.java

+
+    static {
+        MAPPER.setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE);
+        MAPPER.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY);


for consistency shall we add
MAPPER.setPropertyNamingStrategy(PropertyNamingStrategy.SNAKE_CASE);
here?

easbar added 2 commits March 21, 2022 12:55

Serialize enum encoded values, first try

8e70f1b

Serialize and deserialize a list of different encoded values.

9726028

easbar commented Mar 21, 2022

View reviewed changes

core/src/test/java/com/graphhopper/routing/ev/EnumEncodedValueTest.java Outdated Show resolved Hide resolved

Use @JsonTypeInfo -> better

5d36665

easbar commented Mar 21, 2022

View reviewed changes

easbar added 5 commits March 21, 2022 16:44

Use all-arg constructors and make fields final again, not really bett…

74d16d6

…er because of subclass constructors

Revert some changes, support string ev

dd9a6bd

Add helper class to serialize encoded values, actually test something.

aae6b32

Ignore internal fields that will be set by EncodingManager

b581f10

Add version check

d84658c

easbar marked this pull request as ready for review March 21, 2022 17:01

easbar commented Mar 21, 2022

View reviewed changes

otbutz suggested changes Mar 21, 2022

View reviewed changes

easbar added 2 commits March 21, 2022 18:34

Make enum EV fields private again

3d55c5e

Fix StringEncodedValue

a079179

easbar added 2 commits March 22, 2022 09:06

Make constructors package private

4ebf680

Merge branch 'master' into serialize_encoded_values

20449a6

karussell approved these changes Mar 22, 2022

View reviewed changes

Use snake case

2699733

easbar merged commit acd82a8 into master Mar 22, 2022

easbar deleted the serialize_encoded_values branch March 22, 2022 18:12

karussell added this to the 6.0 milestone Mar 23, 2022

easbar mentioned this pull request Jun 19, 2022

Load EncodingManager from properties instead of config #2607

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load encoded values from disk #2542

Load encoded values from disk #2542

easbar commented Mar 21, 2022 •

edited

easbar Mar 21, 2022

easbar Mar 21, 2022

karussell Mar 22, 2022

otbutz Mar 21, 2022

easbar Mar 21, 2022

otbutz Mar 21, 2022

easbar Mar 21, 2022 •

edited

easbar Mar 21, 2022 •

edited

easbar Mar 21, 2022 •

edited

easbar commented Mar 21, 2022

otbutz commented Mar 22, 2022

easbar commented Mar 22, 2022

karussell left a comment

karussell Mar 22, 2022

karussell Mar 22, 2022

Load encoded values from disk #2542

Load encoded values from disk #2542

Conversation

easbar commented Mar 21, 2022 • edited

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

easbar Mar 21, 2022 • edited

Choose a reason for hiding this comment

easbar Mar 21, 2022 • edited

Choose a reason for hiding this comment

easbar Mar 21, 2022 • edited

Choose a reason for hiding this comment

easbar commented Mar 21, 2022

otbutz commented Mar 22, 2022

easbar commented Mar 22, 2022

karussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easbar commented Mar 21, 2022 •

edited

easbar Mar 21, 2022 •

edited

easbar Mar 21, 2022 •

edited

easbar Mar 21, 2022 •

edited