search

westnordost / osm-legal-default-speeds

Infer default legal speed limits from OpenStreetMap tags
https://westnordost.github.io/osm-legal-default-speeds
BSD 3-Clause "New" or "Revised" License
21 stars 9 forks source link

replace regex with indexOf #8

Closed karussell closed 1 year ago

karussell commented 1 year ago

For #7. Not sure how to do performance tests in kotlin. Is there a native alternative for JMH?

Update: although this improves the import for my smaller test PBF it does not seem to improve the memory problems for planet. Update: was a build problem

karussell commented 1 year ago

Obligatory question: Did you test it? I.e. run the unit tests?

Yes :)

Now, I have recently read that copying data is one of those things modern CPUs are really good at

Yeah, normally it is no problem and often the JVM can handle smaller frequently created garbage. But the GraphHopper import is delicate and it might not be the same (as a method is called millions of times). Will profile it again after this change. But the build makes me headaches as sometimes it worked to push it to the local maven repo and sometimes not.

westnordost commented 1 year ago

But the build makes me headaches as sometimes it worked to push it to the local maven repo and sometimes not.

Hm. Maybe the multiplatform setup is delicate... Is there an update for the multiplatform maven plugin?

westnordost commented 1 year ago

Also, I am not sure if publishing to local maven works if a local version with the same version number already exists. Maybe you need to delete the one you already published just then.

westnordost commented 1 year ago

So from my point of view, this could be merged now. Or do you want to also try out asSequence to not create unnecessary new lists/sets/maps in this same PR first?

karussell commented 1 year ago

Sure, feel free to merge it :) (I can't :) )

If you like or want it might be beneficial to setup the benchmark mentioned in #7 before merging, but this is up to you.

Or do you want to also try out asSequence to not create unnecessary new lists/sets/maps in this same PR first?

No, I think it is fine for me to approach this in smaller steps.

Obligatory question: Did you test it? I.e. run the unit tests?

Maybe you activate github actions to run the tests automatically?

westnordost commented 1 year ago

Maybe you activate github actions to run the tests automatically?

It would be prudent to do so... but I am too lazy to do this now. I'd have to read up a bit about github actions first. A PR that enables that however would be welcome :-)

karussell commented 1 year ago

btw: feel free to copy stuff from us https://github.com/graphhopper/graphhopper/blob/master/.github/workflows/build.yml (the node cache is probably not necessary)