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

v1 release planning suggestion #9

Closed
vladimiry opened this issue Jan 10, 2019 · 13 comments
Closed

v1 release planning suggestion #9

vladimiry opened this issue Jan 10, 2019 · 13 comments

Comments

@vladimiry
Copy link
Contributor

I think it would make sense to pre-release v1.0.0-beta1 first in beta npm channel. I'm going to try using it then for a while in this project https://github.com/vladimiry/email-securely-app, so that would be an addition beta testing before final release.

@localvoid
Copy link
Owner

Published: ndx, ndx-index, ndx-query, ndx-compat and ndx-serializable.

@localvoid
Copy link
Owner

Tomorrow I'll try to optimize ndx-serializable data structures. I'll replace trie data structure for inverted index with two arrays: keys and values. gzip should be able to compress common prefixes for keys.

@vladimiry
Copy link
Contributor Author

I will not be able to try ndx with email-securely-app earlier than in a week or few, doing some refactoring and preparations for full-text feature enabling. Besides the initial implementation is not going to use serialization, but there is going to be such need.

@vladimiry
Copy link
Contributor Author

This release https://github.com/vladimiry/email-securely-app/releases/tag/v2.2.0 uses recently published ndx beta version for search. The app doesn't use the serialization yet though. Search has been working great so far. Will provide feedback in a few weeks and then close this issue.

@localvoid
Copy link
Owner

Do you think it would be better if I move all code from ndx-index into ndx, since it is highly unlikely that there will be an alternative document indexing implementation and it is tightly coupled with the data structure that is used in ndx?

Also, as you've noticed ndx-utils is quite bad, so I want to move this functions to ndx-compat and remove this package. In the documentation I'll just use lodash words() function as a tokenizer.

@localvoid
Copy link
Owner

Also, I want to change id property in DocumentDetails to doc because in some use cases this indirection is useless and we can just store direct refs to document objects. It would also require small change in the addDocumentIndex() function, id argument will be replaced with an optional argument to specify custom key for use cases that want to store some unique keys instead of direct refs.

@vladimiry
Copy link
Contributor Author

vladimiry commented Jan 17, 2019

Do you think it would be better if I move all code from ndx-index into ndx

It makes sense to me too.

I want to change id property in DocumentDetails to doc because in some use cases this indirection is useless and we can just store direct refs to document objects

Does it mean index will keep a reference to the original/full document argument passed to addDocumentToIndex? I'd like there will still be an option not storing a reference to a full document, as I don't want full documents to be kept in memory after they got indexed. I keep the original documents in one process and do indexing in another process I just take the search result ids from which and then prepare final search result in the first process. Communicating between processes is done using IPC scenario.

@localvoid
Copy link
Owner

localvoid commented Jan 17, 2019

Does it mean index will keep a reference to the original/full document argument passed to addDocumentToIndex?

Only when id isn't specified, I want to make it as a last optional argument in this function. Data structures will be the same, I'll just rename id to doc.

I'd like there will still be an option not storing a reference to a full document, as I don't want full documents to be kept in memory after they got indexed.

Yes, I understand that such use case is extremely important, I also using it in exactly the same way.

@vladimiry
Copy link
Contributor Author

I want to make it as a last optional argument in this function

Just a note. If a function gets a lot of arguments, especially if some of them are optional, it might wort considering turning arguments to a single object argument as generally going with an object as an argument is a more flexible option and more self-documented.

@localvoid
Copy link
Owner

I don't think that JIT in v8/others will inline this function and because of it escape analysis won't be able to figure out that it can remove this temporary allocation. Don't want additional memory allocation per each invocation since it is a low-level API.

@vladimiry
Copy link
Contributor Author

vladimiry commented Jan 17, 2019

Sure. Thanks for thorough analyzing and approach to building this library. One of the reasons I didn't enable serialization for above-mentioned program with initial search feature release is that indexing and search work quite fast.

@localvoid
Copy link
Owner

Published in the beta channel: ndx, ndx-query, ndx-compat and ndx-serializable.

Decided to keep it simple without any optional arguments, addDocumentToIndex() API hasn't changed.

  • Functions from ndx-index moved to ndx
  • QueryResult property name docId renamed to key. docId implied that it should be some kind of an ID.

@vladimiry
Copy link
Contributor Author

The desktop app mentioned above is working well with the following versions:

  "ndx": "1.0.0-beta.2",
  "ndx-query": "1.0.0-beta.2",
  "ndx-utils": "1.0.0-beta.0",

I didn't notice a memory leaks. Closing the issue as resolved.

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