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

Refactor StringValueStore to use external memory to limit memory usage. #69

Closed
narekgharibyan opened this issue Apr 24, 2018 · 5 comments

Comments

@narekgharibyan
Copy link
Member

JsonValueStore already has this functionality implemented. Probably it's possible to factor out common code and reuse it in both value stores.

@hendrikmuhs
Copy link
Contributor

An option I see is using template specialization (or class inheritance?): The StringValueStore and the JsonValueStore should only differ by 2 operations: encoding and decoding json, everything else should be almost identical.

@narekgharibyan
Copy link
Member Author

To me it sounds like a good application for inheritance, but @amit-cliqz is going to investigate this on upcoming days.

@hendrikmuhs
Copy link
Contributor

hendrikmuhs commented Sep 6, 2018

While working on #93, I realized that the StringValueStore is quite different to the JsonValueStore, so reusage is more complicated. #93 is a step forward, it reuses parts and makes use of external memory.

Some background:

The JsonValueStore has been based on StringValueStore but later been completely rewritten to support compression. As this has been a breaking change JsonValueStore got renamed to JsonValueStoreDeprecated and the new JsonValueStore took it's place using a new enum value. For a while both existed while JsonValueStoreDeprecated got faded out and finally removed somewhen in 0.1 days, only the enum is still there.

I would actually do the same for StringValueStore in order to support compression, this new StringValueStore will - as discussed - share code with JsonValueStore. The old StringValueStore will be renamed and finally removed in 0.4 or 0.5.

@narekgharibyan
Copy link
Member Author

As #93 also implements the functionality this issue requires, I'd suggest to close this one with #93 merge, and later on open a new issue to "add a compression functionality and make use of common code".

@hendrikmuhs
Copy link
Contributor

fixed by #93

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