[jira] Created: (LUCENE-1726) IndexWriter.readerPool create new segmentReader outside of sync block

    [ https://issues.apache.org/jira/browse/LUCENE-1726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12729024#action_12729024 ]

Jason Rutherglen commented on LUCENE-1726:
------------------------------------------

{quote}I don't think that's needed? The core is simply carried
over to the newly cloned reader.{quote}

Right however wouldn't it be somewhat cleaner to sync on core
for all clone operations given we don't want those to occur
(external to IW) at the same time? Ultimately we want core to be
the controller of it's resources rather than the SR being cloned?

I ran the test with the SRMapValue sync code, (4 threads) with
the sync on SR.core in openDocStore for 10 minutes, 2 core
Windows XML laptop Java 6.14 and no errors. Then same with 2
threads for 5 minutes and no errors. I'll keep on running it to
see if we can get an error.

I'm still a little confused as to why we're going to see the bug
if readerPool.get is syncing on the SRMapValue. I guess there's
a slight possibility of the error, and perhaps a more randomized
test would produce it.

> IndexWriter.readerPool create new segmentReader outside of sync block
> ---------------------------------------------------------------------
>
>                 Key: LUCENE-1726
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1726
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Index
>    Affects Versions: 2.4.1
>            Reporter: Jason Rutherglen
>            Assignee: Michael McCandless
>            Priority: Trivial
>             Fix For: 3.1
>
>         Attachments: LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.trunk.test.patch
>
>   Original Estimate: 48h
>  Remaining Estimate: 48h
>
> I think we will want to do something like what field cache does
> with CreationPlaceholder for IndexWriter.readerPool. Otherwise
> we have the (I think somewhat problematic) issue of all other
> readerPool.get* methods waiting for an SR to warm.
> It would be good to implement this for 2.9.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@...
For additional commands, e-mail: java-dev-help@...

     [ https://issues.apache.org/jira/browse/LUCENE-1726?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Michael McCandless updated LUCENE-1726:
---------------------------------------

    Attachment: LUCENE-1726.patch

Attached new patch (the patch is worse than it looks, because many
things moved into the CoreReaders class):

  * Moved more stuff into CoreReaders (fieldInfos, dir, segment, etc.)
    and moved methods down as well (ctor, openDocStores, decRef).

  * Made members final when possible, else synchronized access to
    getting them (to avoid running amok of JMM).

{quote}
Right however wouldn't it be somewhat cleaner to sync on core
for all clone operations given we don't want those to occur
(external to IW) at the same time? Ultimately we want core to be
the controller of it's resources rather than the SR being cloned?
{quote}

In fact, I'm not sure why cloning/reopening a segment needs to be
synchronized at all.

Sure it'd be weird for an app to send multiple threads down,
attempting to reopen/clone the same SR or core at once, but from
Lucene's standpoint there's nothing "wrong" with doing so, I think?

(Though, DirectoryReader does need its sync when its transferring the
write lock due to reopen on a reader w/ pending changes).

{quote}
I ran the test with the SRMapValue sync code, (4 threads) with
the sync on SR.core in openDocStore for 10 minutes, 2 core
Windows XML laptop Java 6.14 and no errors. Then same with 2
threads for 5 minutes and no errors. I'll keep on running it to
see if we can get an error.
{quote}

You could try inserting a testPoint (see the other testPoints in
IndexWriter) after the SRMapValue is pulled from the hash but before
we check if its reader is null, and then modify the threads in your
test to randomly yield on that testPoint (by subclassing IW)?  Ie
"exacerbate" the path that exposes the hazard.

{quote}
I'm still a little confused as to why we're going to see the bug
if readerPool.get is syncing on the SRMapValue. I guess there's
a slight possibility of the error, and perhaps a more randomized
test would produce it.
{quote}

The hazard exists because there's a time when no synchronization is
held.  Ie, you retrieve SRMapValue from the hash while sync'd on
ReaderPool.  You then leave that sync entirely (this is where hazard
comes in), and next you sync on the SRMapValue.  Another thread can
sneak in and close the SRMapValue.reader during the time that no sync
is held.

> IndexWriter.readerPool create new segmentReader outside of sync block
> ---------------------------------------------------------------------
>
>                 Key: LUCENE-1726
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1726
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Index
>    Affects Versions: 2.4.1
>            Reporter: Jason Rutherglen
>            Assignee: Michael McCandless
>            Priority: Trivial
>             Fix For: 3.1
>
>         Attachments: LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.trunk.test.patch
>
>   Original Estimate: 48h
>  Remaining Estimate: 48h
>
> I think we will want to do something like what field cache does
> with CreationPlaceholder for IndexWriter.readerPool. Otherwise
> we have the (I think somewhat problematic) issue of all other
> readerPool.get* methods waiting for an SR to warm.
> It would be good to implement this for 2.9.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@...
For additional commands, e-mail: java-dev-help@...

    [ https://issues.apache.org/jira/browse/LUCENE-1726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12730009#action_12730009 ]

Jason Rutherglen commented on LUCENE-1726:
------------------------------------------

{quote}try inserting a testPoint (see the other testPoints in
IndexWriter) after the SRMapValue is pulled from the hash but before
we check if its reader is null{quote}

I added the test point, but tested without the yield, the method
itself was enough of a delay to expose the exception.  

> IndexWriter.readerPool create new segmentReader outside of sync block
> ---------------------------------------------------------------------
>
>                 Key: LUCENE-1726
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1726
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Index
>    Affects Versions: 2.4.1
>            Reporter: Jason Rutherglen
>            Assignee: Michael McCandless
>            Priority: Trivial
>             Fix For: 3.1
>
>         Attachments: LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.trunk.test.patch
>
>   Original Estimate: 48h
>  Remaining Estimate: 48h
>
> I think we will want to do something like what field cache does
> with CreationPlaceholder for IndexWriter.readerPool. Otherwise
> we have the (I think somewhat problematic) issue of all other
> readerPool.get* methods waiting for an SR to warm.
> It would be good to implement this for 2.9.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@...
For additional commands, e-mail: java-dev-help@...

    [ https://issues.apache.org/jira/browse/LUCENE-1726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12730010#action_12730010 ]

Jason Rutherglen commented on LUCENE-1726:
------------------------------------------

I haven't really figured out a clean way to move the reader
creation out of the reader pool synchronization. It turns out to
be somewhat tricky, unless we redesign our synchronization.

One thing that came to mind is passing a lock object to SR's
core (which would be the same lock on SRMapValue), which the
incref/decref could sync on as well. Otherwise we've got
synchronization in many places, IW, IW.readerPool, SR, SR.core.
It would seem to make things brittle? Perhaps listing out the
various reasons we're synchronizing, to see if we can
consolidate some of them will help?

> IndexWriter.readerPool create new segmentReader outside of sync block
> ---------------------------------------------------------------------
>
>                 Key: LUCENE-1726
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1726
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Index
>    Affects Versions: 2.4.1
>            Reporter: Jason Rutherglen
>            Assignee: Michael McCandless
>            Priority: Trivial
>             Fix For: 3.1
>
>         Attachments: LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.trunk.test.patch
>
>   Original Estimate: 48h
>  Remaining Estimate: 48h
>
> I think we will want to do something like what field cache does
> with CreationPlaceholder for IndexWriter.readerPool. Otherwise
> we have the (I think somewhat problematic) issue of all other
> readerPool.get* methods waiting for an SR to warm.
> It would be good to implement this for 2.9.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@...
For additional commands, e-mail: java-dev-help@...

    [ https://issues.apache.org/jira/browse/LUCENE-1726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12730030#action_12730030 ]

Jason Rutherglen commented on LUCENE-1726:
------------------------------------------

Another idea is instantiate the SR.core.ref outside of the
IW.readerPool, and pass it into the newly created reader. Then
when we obtain SRMapValue, incref so we're keeping track of it's
usage, which (I believe) should be inline with the normal usage
of SR.ref (meaning don't close the reader if SRMV is checked
out). This way we know when the SRMV is in use and different
threads don't clobber each other creating and closing SRs using
readerPool.

> IndexWriter.readerPool create new segmentReader outside of sync block
> ---------------------------------------------------------------------
>
>                 Key: LUCENE-1726
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1726
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Index
>    Affects Versions: 2.4.1
>            Reporter: Jason Rutherglen
>            Assignee: Michael McCandless
>            Priority: Trivial
>             Fix For: 3.1
>
>         Attachments: LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.trunk.test.patch
>
>   Original Estimate: 48h
>  Remaining Estimate: 48h
>
> I think we will want to do something like what field cache does
> with CreationPlaceholder for IndexWriter.readerPool. Otherwise
> we have the (I think somewhat problematic) issue of all other
> readerPool.get* methods waiting for an SR to warm.
> It would be good to implement this for 2.9.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@...
For additional commands, e-mail: java-dev-help@...

    [ https://issues.apache.org/jira/browse/LUCENE-1726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12730272#action_12730272 ]

Michael McCandless commented on LUCENE-1726:
--------------------------------------------

{quote}
I haven't really figured out a clean way to move the reader
creation out of the reader pool synchronization. It turns out to
be somewhat tricky, unless we redesign our synchronization.
{quote}

Maybe we should simply hold off for now?

I don't think this sync is costing much in practice, now.
Ie, IndexReader.open is not concurrent when opening its segments; nor
would we expect multiple threads to be calling IndexWriter.getReader
concurrently.

There is a wee bit of concurrency we are preventing, ie for a merge or
applyDeletes to get a reader just as an NRT reader is being opened,
but realistically 1) that's not very costly, and 2) we can't gain that
concurrency back anyway because we synchronize on IW when opening the
reader.

> IndexWriter.readerPool create new segmentReader outside of sync block
> ---------------------------------------------------------------------
>
>                 Key: LUCENE-1726
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1726
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Index
>    Affects Versions: 2.4.1
>            Reporter: Jason Rutherglen
>            Assignee: Michael McCandless
>            Priority: Trivial
>             Fix For: 3.1
>
>         Attachments: LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.trunk.test.patch
>
>   Original Estimate: 48h
>  Remaining Estimate: 48h
>
> I think we will want to do something like what field cache does
> with CreationPlaceholder for IndexWriter.readerPool. Otherwise
> we have the (I think somewhat problematic) issue of all other
> readerPool.get* methods waiting for an SR to warm.
> It would be good to implement this for 2.9.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@...
For additional commands, e-mail: java-dev-help@...

    [ https://issues.apache.org/jira/browse/LUCENE-1726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12730424#action_12730424 ]

Jason Rutherglen commented on LUCENE-1726:
------------------------------------------

I was thinking the sync on all of readerPool could delay someone
trying to call IW.getReader who would wait for a potentially
large new segment to be warmed. However because IW.mergeMiddle
isn't loading the term index, IW.getReader will pay the cost of
loading the term index. So yeah, it doesn't seem necessary.

> IndexWriter.readerPool create new segmentReader outside of sync block
> ---------------------------------------------------------------------
>
>                 Key: LUCENE-1726
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1726
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Index
>    Affects Versions: 2.4.1
>            Reporter: Jason Rutherglen
>            Assignee: Michael McCandless
>            Priority: Trivial
>             Fix For: 3.1
>
>         Attachments: LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.patch, LUCENE-1726.trunk.test.patch
>
>   Original Estimate: 48h
>  Remaining Estimate: 48h
>
> I think we will want to do something like what field cache does
> with CreationPlaceholder for IndexWriter.readerPool. Otherwise
> we have the (I think somewhat problematic) issue of all other
> readerPool.get* methods waiting for an SR to warm.
> It would be good to implement this for 2.9.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@...
For additional commands, e-mail: java-dev-help@...