You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2020/02/21 14:50:09 UTC
[GitHub] [incubator-daffodil] stevedlawrence opened a new pull request #325:
Fix non-thread safe code in checking for local delimiters
stevedlawrence opened a new pull request #325: Fix non-thread safe code in checking for local delimiters
URL: https://github.com/apache/incubator-daffodil/pull/325
As an optimization, when we add a DFADelimiter to the stack of in-scope
delimiters, we mutate the DFADelimiter to store its index in that stack.
This stored index allows us to quickly determine if a found delimiter is
local or remote by just comparing the index to first local DFA index.
Although these DFADelimiters are mutated, which is the sort of thing
that is usually not thread safe for us, it actually is thread safe
because each instance of a DFADelmiter always ends up in the same index.
So different threads might mutate the same DFADelimiter, but they always
mutated it to the same value.
However, with the recent change to avoid copying, the same DFADelimiter
can now be shared at different stack positions in a parse. This causes
mutable state in the DFADelimiter to differ, and now different threads
can mutate that state with different values and cause random failures.
This commit removes this mutable state and instead just searches the
in-scope delimiters for a matched delimiter to determine if it is local
or remote. Although this removes the optimization, initial performance
testing shows no real difference.
DAFFODIL-2284
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on issue #325: Fix
non-thread safe code in checking for local delimiters
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on issue #325: Fix non-thread safe code in checking for local delimiters
URL: https://github.com/apache/incubator-daffodil/pull/325#issuecomment-594013141
The deeply nested schema really just tested performance, which I generally prefer to avoid putting in a repo since it just slows things down and doesn't really test functionality. We already have plenty of nested separator tests already in the repo for functionality, so I don't think it provides anything that we don't already have.
And this test wouldn't have even found this error since it required threading and a unlucky timing. We probably need to come up with some other solution to better test threading issues. Not sure what that would look like though. Right now we just rely on threaded nightlies.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull
request #325: Fix non-thread safe code in checking for local delimiters
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #325: Fix non-thread safe code in checking for local delimiters
URL: https://github.com/apache/incubator-daffodil/pull/325#discussion_r385308852
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/DelimiterParsers.scala
##########
@@ -51,27 +51,22 @@ class DelimiterTextParser(
override val nom = delimiterType.toString
- private def containsLocalMatch(delimiters: ArrayBuffer[DFADelimiter], state: PState): Boolean = {
- var i = delimiters.length - 1
+ private def containsLocalMatch(matchedDelimiters: ArrayBuffer[DFADelimiter], state: PState): Boolean = {
+ val localIndexStart = state.mpstate.delimitersLocalIndexStack.top
+ val inScopeDelimiters = state.mpstate.delimiters
+
+ var foundLocalDFAIndex = -1
+
// scan matches in reverse. local matches are going to be at the end
- while (i >= 0) {
- val dfa = delimiters(i)
- if (dfa.indexInDelimiterStack < state.mpstate.delimitersLocalIndexStack.top) {
- // we found a remote delim. since all remote delimiters are first, and
- // we are traversing this in reverse, we know all remaining delims are
- // remotes, so we can return early
- return false
- }
- // note, it is possible for dfaIndex to be greater than the length of the
- // found delimiters. This just means a found delimiter went out of scope
- // (and thus, not a local delimiter)
- if (dfa.delimType == delimiterType && dfa.indexInDelimiterStack < state.mpstate.delimiters.length) {
- // found a local delim
- return true
+ var i = matchedDelimiters.length - 1
+ while (foundLocalDFAIndex < 0 && i >= 0) {
+ val dfa = matchedDelimiters(i)
+ if (dfa.delimType == delimiterType) {
Review comment:
I did as you suggested and created a schema with deeply nested separators and some pretty ugly data that would find lots of remote delimiters and requiring lots of scanning of the delimter stack. This test didn't find any noticeable performance differences. At most it was just 1-2% slower, but that's within the margin of error for java, and I had test runs that were just as performant as before this change.
I also inserted some time trackers, and it looks like only about 6% of the total parse time is spent in the parts of DelimiterStackParser that isn't doing actual delimiter scanning (so that includes creating delimiter iterators, determining local/remote which is what this change affects, skipping matched characters, creating ParseErrors, etc). 41% of the parse is spent in the actual delimiter scanning part, and 30% in the sequence parser stuff, and miscellaneous other stuff.
So I think this index stuff was just a premature optimization--the bulk of time is spent elsewhere.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull
request #325: Fix non-thread safe code in checking for local delimiters
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #325: Fix non-thread safe code in checking for local delimiters
URL: https://github.com/apache/incubator-daffodil/pull/325#discussion_r382937460
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/DelimiterParsers.scala
##########
@@ -51,27 +51,22 @@ class DelimiterTextParser(
override val nom = delimiterType.toString
- private def containsLocalMatch(delimiters: ArrayBuffer[DFADelimiter], state: PState): Boolean = {
- var i = delimiters.length - 1
+ private def containsLocalMatch(matchedDelimiters: ArrayBuffer[DFADelimiter], state: PState): Boolean = {
+ val localIndexStart = state.mpstate.delimitersLocalIndexStack.top
+ val inScopeDelimiters = state.mpstate.delimiters
+
+ var foundLocalDFAIndex = -1
+
// scan matches in reverse. local matches are going to be at the end
- while (i >= 0) {
- val dfa = delimiters(i)
- if (dfa.indexInDelimiterStack < state.mpstate.delimitersLocalIndexStack.top) {
- // we found a remote delim. since all remote delimiters are first, and
- // we are traversing this in reverse, we know all remaining delims are
- // remotes, so we can return early
- return false
- }
- // note, it is possible for dfaIndex to be greater than the length of the
- // found delimiters. This just means a found delimiter went out of scope
- // (and thus, not a local delimiter)
- if (dfa.delimType == delimiterType && dfa.indexInDelimiterStack < state.mpstate.delimiters.length) {
- // found a local delim
- return true
+ var i = matchedDelimiters.length - 1
+ while (foundLocalDFAIndex < 0 && i >= 0) {
+ val dfa = matchedDelimiters(i)
+ if (dfa.delimType == delimiterType) {
Review comment:
We should consider constructing a really pathological schema with nest after nest after nest of separated sequences so that this while loop actually has to search down a bit. Just so we can understand how bad it has to be before this matters at all. It's possible that for an array, where the same delimiter is being searched repeatedly that this loop does not need to iterate anew for each one. But that's an optimization I would put in only if we had measurements.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence merged pull request #325: Fix
non-thread safe code in checking for local delimiters
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #325: Fix non-thread safe code in checking for local delimiters
URL: https://github.com/apache/incubator-daffodil/pull/325
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services