equivalent to java's Lucene patch 5644: switch to simpler LIFO thread to ThreadState allocator d…

[vvdb]

…uring indexing”. Technically, this is something from releases/lucene-solr/4.8.1, but profiling indicates it makes a huge difference in multithreaded scenarios

[synhershko]

this seems unrelated?

[synhershko]

Thanks, that's an interesting one.

First, there's a lot of whitespace noise - can you remove it please? (align with the project's whitespace specs)

Second, assuming we have no threading issues at the moment, how can we be sure this doesn't introduce any? cc @NightOwl888

[vvdb]

> align with the project's whitespace specs

I’ve been trying to find the project’s code formatting document, but was unsuccessful.

> Second, assuming we have no threading issues at the moment, how can we be sure this doesn't introduce any?

We can be sure of nothing, since certainty is only as strong as the assumptions it is based on. However: - the tests have been appropriately changed and use the new code, and they all pass. - On a more personal level (even though it means nothing since you have to take my unverifiable word for it), this code has been in production use these past 3 months on a 28-core machine, all of them writing to a single IndexWriter for about a hundred million documents, repeatedly. No problems were ever seen. These things don’t show the absence of problems, but make me reasonably confident that the code is sound. Vincent From: Itamar Syn-Hershko [mailto:notifications@github.com] Sent: Tuesday, June 13, 2017 9:09 AM To: apache/lucenenet <lucenenet@noreply.github.com> Cc: Van Den Berghe, Vincent <Vincent.VanDenBerghe@bvdinfo.com>; Author <author@noreply.github.com> Subject: Re: [apache/lucenenet] LUCENE-5644: switch to simpler LIFO thread to ThreadState allocator d… (#208) Thanks, that's an interesting one. First, there's a lot of whitespace noise - can you remove it please? (align with the project's whitespace specs) Second, assuming we have no threading issues at the moment, how can we be sure this doesn't introduce any? cc @NightOwl888<https://github.com/nightowl888> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#208 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ARBXpku1AqOToF4KGaoBuQG8Pu83axMhks5sDjV4gaJpZM4N4A9a>.

[NightOwl888]

Second, assuming we have no threading issues at the moment, how can we be sure this doesn't introduce any? cc @NightOwl888

@synhershko

First of all, we still have some threading issues. The Lucene.Net.Store.TestLockFactory.TestStressLocksNativeFSLockFactory test and Lucene.Net.Store.TestLockFactory.TestStressLocks test still fail randomly, and there may be others (last I checked there were 2 tests that failed only when the MMapDirectory was randomly selected).

Secondly, this change is part of the Lucene 4.8.1 patch. Much of this port (including most or all of Analysis.Common) is from 4.8.1 already, so we should probably try to include the rest of the fixes to get us all the way to 4.8.1, especially if they have performance benefits.

@vvdb

If we are going to work toward 4.8.1 on a gradual basis, we should probably include a comment at the top of each changed file indicating the version compatibility level so we don't have to check the entire file to see if it is up to speed.

// Version compatibility level: 4.8.1

As for getting all the way there, I have outlined a procedure here for upgrading Lucene.Net to 4.8.1, which should be a lot quicker and easier than porting it from scratch again. Of course, it requires you to have a text comparison tool that allows you to filter out "unimportant changes" such as Beyond Compare. This will ensure we get all of the changes between the old file and new file and port them into the .NET file.

It would be better to have a tool that allows you to ignore simple code formatting changes (such as changing the curly bracket from the same line to the following line of a function or if statement), but to my knowledge a tool like that doesn't exist.

[NightOwl888]

@vvdb

I would like to add this fix to the next release. Are you planning to submit another pull request?

[vvdb]

I’m not planning to submit another pull request at this time. Feel free to take whatever you need from my repository: you’re better at this than I am. Vincent From: Shad Storhaug [mailto:notifications@github.com] Sent: Tuesday, June 27, 2017 10:27 AM To: apache/lucenenet <lucenenet@noreply.github.com> Cc: Van Den Berghe, Vincent <Vincent.VanDenBerghe@bvdinfo.com>; Mention <mention@noreply.github.com> Subject: Re: [apache/lucenenet] LUCENE-5644: switch to simpler LIFO thread to ThreadState allocator d… (#208) @vvdb<https://github.com/vvdb> I would like to add this fix to the next release. Are you planning to submit another pull request? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#208 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ARBXprqH0s0Tjj1dhQ4f_ryNc6H2vCZEks5sILy9gaJpZM4N4A9a>.

[dsmiley]

Guys, the PR title here references LUCENE-5644 and this trigger's ASF JIRA-GitHub integration to link the conversation here to comments on that old issue. Can you please edit the PR title?