Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

using struct instead of sealed class? #93

Open
Eraclys opened this issue Nov 15, 2022 · 6 comments
Open

using struct instead of sealed class? #93

Eraclys opened this issue Nov 15, 2022 · 6 comments

Comments

@Eraclys
Copy link

Eraclys commented Nov 15, 2022

Hello @pocketken ,

Just wondering if you have considered using struct instead of a sealed class for H3Index to avoid memory allocations on the heap and make better use of processor caching?

I've been considering making a PR but you may already have investigated the subject?

Thanks

@pocketken
Copy link
Owner

Hi @Eraclys,

I think I may have started going down that path at one point, but I can't remember why I stopped -- I think the particular microbenchmarks I was doing at the time were actually showing a drop in performance when I switched? It was very early on, though and very likely that whatever I was testing was extremely narrow in scope.

So, all that being said, if you are open to throwing together a PR I am definitely interested in taking a look at it. I've been swamped with work the past few months, but I should have some time coming up in the next few weeks to give this project a bit of TLC -- in particular I need to get #86 done so that the library is producing equivalent results to upstream release 4.0.1. Maybe we could get a release out that includes both?

Thanks!

@Eraclys
Copy link
Author

Eraclys commented Nov 18, 2022

Hi @pocketken,

Thank you for your reply.

I'll try and have a look whenever I get a chance.

@pocketken
Copy link
Owner

Hi @Eraclys, did you happen to have a chance to do any playing around with this idea yourself?

I'm hoping to start working on the updates required to align with 4.1.0 (#102) at some point in the next week or so. I can't guarantee anything will come out of it, but I am planning on spending a little bit of time investigating this idea as well while I'm in here. If you've done any preliminary work / scoping, let me know!

@Eraclys
Copy link
Author

Eraclys commented Jan 27, 2023

Hi @pocketken,

I started looking into it but other things came up.
It looked feasible, the main issue was the tests comparison by packages since the types no longer match.
Let me know how it goes.

@pocketken
Copy link
Owner

Yeah; means we'll have to do separate benchmark projects for each and manually compare.

pocketken added a commit that referenced this issue Mar 3, 2023
* fix GH-86: change Vec2d.Intersect to use double instead of float
* fix GH-98: add net7.0 TFM
* some work on GH-93: convert a few model classes to struct, still lots
  to do especially to avoid API breakage
* replace LeadingZeros polyfill with the actual code from
  BitConverter.LeadingZeroCount
* attempt at optimizing H3Index.LeadingNonZeroDirection
* optimize H3Index.RotatePentagonClockwise and
  H3Index.RotatePentagonCounterClockwise
* update benchmarks to focus on comparison between 4.0.0 and eventual
  4.1.0 release
@pocketken
Copy link
Owner

Haven't forgotten about this @Eraclys have just been swamped. Actually I need a new word for swamped, as it does not adequately describe the past few months at all... anyway! I will finally get back to looking at this in the next couple of weeks now that things are finally calming down a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants