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

Native typesize::TypeSize implementation? #39

Open
GnomedDev opened this issue Sep 10, 2024 · 8 comments · May be fixed by #42
Open

Native typesize::TypeSize implementation? #39

GnomedDev opened this issue Sep 10, 2024 · 8 comments · May be fixed by #42
Labels
enhancement New feature or request
Milestone

Comments

@GnomedDev
Copy link
Contributor

Hey, I'm the developer of typesize and currently it has a feature locked inaccurate implementation due to privacy, but due to the breaking release needed for #34 to be published that implementation will be pointing to the wrong version.

Would a PR to move the TypeSize implementation into mini-moka under a feature gate be accepted so I don't have to maintain the implementation in typesize? This is similar to xacrimon/dashmap#308.

This would lead to me closing #18 if accepted.

@tatsuya6502
Copy link
Member

Would a PR to move the TypeSize implementation into mini-moka under a feature gate be accepted so I don't have to maintain the implementation in typesize? This is similar to xacrimon/dashmap#308.

Hi. Yes, that would be the best way. Such a PR should definitely be accepted. (and sorry for not making any progress on #18)

Will you please create a PR for this? It does not have to be perfect, but something that I can start with would be great. I will merge it and then improve it before next release of mini-moka.

You could implement the TypeSize trait for the following internal data structures. That would be very helpful:

  • struct FrequencySketch:
    pub(crate) struct FrequencySketch {
    sample_size: u32,
    table_mask: u32,
    table: Box<[u64]>,
    size: u32,
    }
  • struct Deque<T>:
    pub(crate) struct Deque<T> {
    region: CacheRegion,
    len: usize,
    head: Option<NonNull<DeqNode<T>>>,
    tail: Option<NonNull<DeqNode<T>>>,
    cursor: Option<DeqCursor<T>>,
    marker: PhantomData<Box<DeqNode<T>>>,
    }

Note that the main branch has something not ready for a release yet. So please create the PR against the v0.11.x branch, which is the default branch now.

Thank you.

@tatsuya6502 tatsuya6502 added the enhancement New feature or request label Sep 10, 2024
@tatsuya6502 tatsuya6502 added this to the v0.11.0 milestone Sep 10, 2024
@tatsuya6502
Copy link
Member

Oops. v0.11.x does not have your previous PR #34 yet. Let me work on it today (#38).

@tatsuya6502
Copy link
Member

OK. Now the v0.11.x branch has your changes from #34 and depends on DashMap 6. Please pull v0.11.x branch at 732ff1f.

@GnomedDev
Copy link
Contributor Author

Sure, I'll work on this later today if I have the energy after work.

@GnomedDev
Copy link
Contributor Author

Looks like this is a bit more of a pain than expected, because there are so many types and third party crates used, I'll get my current stuff pushed as a draft and work on it more later.

@GnomedDev GnomedDev linked a pull request Sep 10, 2024 that will close this issue
@tatsuya6502
Copy link
Member

Thank you so much for working on it. Please take your time.

It will be challenging to get the exact size because of heavy usages of Arc and complex code caused by eventual consistency nature of cache policy data structures (e.g. the MPSC channels and a bunch of locks).

I think many use cases will not need the exact size, but an approximate size will be enough? Maybe we can last resort to it if getting the exact size is too difficult?

For example:

  • pre-allocated capacities (e.g. for those fixed-sized channels and frequency sketch)
  • + number of cached entries × average memory overhead per cached entry
  • + total size of cached keys
  • + total size of cached values
  • (ignore small data structures such as the house keeper because they do not contribute to the total size very much)

@GnomedDev
Copy link
Contributor Author

Once I get the primitives used sorted out, it should be as easy as adding a bunch of derives, it's just that I didn't expect this crate to contain RwLock wrappers and other stuff that I personally would have put in another crate to improve build time (more parallelism) and maintainability.

@tatsuya6502
Copy link
Member

It sounds great! Thank you for working on it.

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

Successfully merging a pull request may close this issue.

2 participants