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

Adds ability to disable caching in DirectoryTaxonomyReader #238

Closed
wants to merge 1 commit into from

Conversation

rohitkumbhar
Copy link

DirectoryTaxonomyReader depends on LurchTable to cache ordinal and
FacetLabel mapping. However, LurchTable causes a SIGSEGV on ARM64.
To work around this crash, I've added the ability to disable the use
of cache in DirectoryTaxonomyReader.

Related issue for changing LurchTable (if at all possible): NightOwl888/J2N#3

DirectoryTaxonomyReader depends on LurchTable to cache ordinal and
FacetLabel mapping. However, LurchTable causes a SIGSEGV on ARM64.
To work around this crash, I've added the ability to disable the use
of cache in DirectoryTaxonomyReader.
@NightOwl888
Copy link
Contributor

Thanks for the report and the PR.

I am not sure if disabling the cache is the right fix for everyone on Xamarin.iOS, as that will likely negatively affect performance. On the other hand, maybe such a feature isn't entirely a bad thing, so please leave this PR open for now.

However, since it seems that the fix from LUCENENET-602 isn't working in every case (even after we had confirmation it worked for others), perhaps it is time to take a different approach.

  • Option 1: There was a report that changing the structs to classes in LurchTable will fix the problem. We could create a target for Xamarin.iOS on J2N and only apply the patch only to that target with conditional compilation.

  • Option 2: There was a report that swapping in LruCache instead of LurchTable will also work. We could create a target for Xamarin.iOS on Lucene.Net.Facet and Lucene.Net.QueryParser and only apply the patch only to that target with conditional compilation.

If you are willing to do the testing and submit a PR with either of those solutions, it would be much appreciated. Ideally, we would have it setup to run tests on Xamarin.iOS, too, but we can probably get by without those tests if you don't have the time to get them working.

Also, if attempting #1, it would be helpful to know whether the fix works without having to target Xamarin.iOS directly in Lucene.Net.Facet and Lucene.Net.QueryParser.

@rohitkumbhar
Copy link
Author

Hi! Thank you for checking this. As-is, the PR will not disable cache and existing performance will not be affected. I set the CacheDisabled to true explicitly in my code to avoid the crash. I'll look at the linked options as well.

@NightOwl888
Copy link
Contributor

Thanks for the PR. If we have any additional reports of problems with LurchTable, I think a better option would be to add constructor overloads of DirectoryTaxonomyWriter that accept an abstract factory so the LruDictionary implementation can be swapped with a custom subclass. However, since we have not received any other reports of crashes on Xamarin.iOS.

I realize I already mentioned it in NightOwl888/J2N#3, but just reporting it here so it is in the Lucene.NET mail archive for reference. I have also closed #242, which is related.

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

Successfully merging this pull request may close these issues.

2 participants