[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #332: Refactor isHidden
mbeckerle commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389908595
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ModelGroupGrammarMixin.scala
##
@@ -68,9 +69,16 @@ trait ModelGroupGrammarMixin
case c: ChoiceTermBase => DelimiterStackCombinatorChoice(c, content)
case s: SequenceTermBase => DelimiterStackCombinatorSequence(s,
content)
}
- } else { groupContentDef }
+ } else {
+groupContent
+ } //TODO: confirm this works; changed from groupContentDef
-finalContent
+ val mgrd = self.modelGroupRuntimeData
+ if (mgrd.isHidden) {
Review comment:
Good point. This is the schema compiler, so we shouldn't be asking the
runtime data objects, but the DSOM object.
The schema compiler creates the runtime data objects, but shouldn't use
them. What they contain is in service of the runtime.
Interestingly, it's possible that ModelGroupRuntimeData doesn't need an
isHidden member. Whether the parser/unparser is traversing a hidden group ref
or not is represented by way of there being a HiddenGroupParser/Unparser
constructed. So possibly we don't even need ModelGroupRuntimeData.isHidden.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #332: Refactor isHidden
mbeckerle commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389901113
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##
@@ -1497,6 +1482,17 @@ class InteractiveDebugger(runner:
InteractiveDebuggerRunner, eCompilers: Express
}
}
+ object InfoHidden extends DebugCommand with DebugCommandValidateZeroArgs
{
+val name = "hidden"
+override lazy val short = "hid"
+val desc = "display whether or not we're within a hidden state"
Review comment:
"... within a hidden group." would better match DFDL spec terminology.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #332: Refactor isHidden
mbeckerle commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389898693
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/SequenceTermRuntime1Mixin.scala
##
@@ -76,7 +77,8 @@ trait ChoiceBranchImpliedSequenceRuntime1Mixin { self:
ChoiceBranchImpliedSequen
None,
Maybe.Nope,
Maybe.Nope,
- Maybe.Nope)
+ Maybe.Nope,
+ isHidden)
Review comment:
This should always be false here. I.e., a ChoiceBranchImpliedSequence cannot
be created for a group ref. So you can write
```
{
Assert.invariant(!isHidden)
false
}
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #332: Refactor isHidden
mbeckerle commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389899025
##
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ChoiceAndOtherVariousUnparsers.scala
##
@@ -29,69 +29,62 @@ import org.apache.daffodil.util.MaybeInt
class ChoiceCombinatorUnparser(
mgrd: ModelGroupRuntimeData,
eventUnparserMap: Map[ChoiceBranchEvent, Unparser],
- choiceLengthInBits: MaybeInt)
+ choiceLengthInBits: MaybeInt,
+ optDefaultableBranchUnparser: Option[Unparser],
+ firstBranchUnparser: Unparser)
extends CombinatorUnparser(mgrd)
with ToBriefXMLImpl {
override def nom = "Choice"
override lazy val runtimeDependencies = Vector()
- override lazy val childProcessors = eventUnparserMap.map { case (k, v) => v
}.toSeq.toVector
+ override lazy val childProcessors = (eventUnparserMap.map { case (k, v) => v
}.toSeq ++ optDefaultableBranchUnparser).toVector
Review comment:
line too long. Please break at the ++
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #332: Refactor isHidden
mbeckerle commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389890072
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ModelGroupGrammarMixin.scala
##
@@ -68,9 +69,16 @@ trait ModelGroupGrammarMixin
case c: ChoiceTermBase => DelimiterStackCombinatorChoice(c, content)
case s: SequenceTermBase => DelimiterStackCombinatorSequence(s,
content)
}
- } else { groupContentDef }
+ } else {
+groupContent
+ } //TODO: confirm this works; changed from groupContentDef
Review comment:
Can we remove this TODO? I think we know this works now.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #332: Refactor isHidden
mbeckerle commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389894066
##
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/Term.scala
##
@@ -732,9 +728,8 @@ trait Term
final def possibleFirstChildElementsInInfoset: Seq[ElementBase] =
LV('possibleFirstChildElementsInInfoset) {
val pfct = possibleFirstChildTerms
val firstChildren = pfct.flatMap {
- case e: ElementBase if e.isHidden => Nil
case e: ElementBase => Seq(e)
- case s: SequenceTermBase if s.isHidden => Nil
+ case gr: GroupRef if gr.asModelGroup.isHidden => Nil
Review comment:
The more I read this code the more I want to suggest changing this member
name from isHidden to isHiddenGroupRef.
That seems easier than the other clarifying change which would be to move
isHidden onto GroupRef and remove it from ModelGroup.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389894439
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/ChoiceCombinator.scala
##
@@ -217,46 +217,49 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
}
override lazy val unparser: Unparser = {
-if (!ch.isHidden) {
- val eventRDMap = ch.choiceBranchMap
- val eventUnparserMap = eventRDMap.flatMap {
-case (cbe, rd) =>
- // if we don't find a matching RD for a term that's probably
- // because the term is an empty sequence or empty choice (which do
happen
- // and we even have tests for them). Since those can never be chosen
by
- // means of an element event, they don't appear in the map.
- val altGram = alternatives.find { alt =>
-val crd = alt.context.runtimeData
-val found = crd =:= rd
-found
- }
- altGram.map { ag => (cbe, ag.unparser) }
- }
- val mapValues = eventUnparserMap.map { case (k, v) => v
}.toSeq.filterNot(_.isEmpty)
- if (mapValues.isEmpty)
-new NadaUnparser(null)
- else {
-new ChoiceCombinatorUnparser(ch.modelGroupRuntimeData,
eventUnparserMap, choiceLengthInBits)
- }
-} else {
- // Choices inside a hidden group ref are slightly different because we
- // will never see events for any of the branches. Instead, we will just
- // always pick the branch in which every thing is defaultble or OVC. It
- // is a warning if more than one of those branches exist. It is an SDE if
- // such a branch does not exist, which is detected elsewhere
-
+val optDefaultableBranchUnparser = if (ch.isHidden) {
Review comment:
Does this logic need to be in the runtime unparsers? We can only know if a
choice is hidden if it is directly referenced by a hidden group ref. But if a
choice is a child of a hidden group ref then this can't know if it's hidden or
not.
Do we need to just always determine a defaultable branch and pass that into
the choice combiantor, and then only use that if we determine we are hidden at
runtime?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389898011
##
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/HiddenGroupCombinatorUnparser.scala
##
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.processors.unparsers
+
+import org.apache.daffodil.processors.ModelGroupRuntimeData
+
+/**
+ * The purpose of this combinator is to increment/decrement the depth counter
+ * for hidden groups, and call the group's unparser. As we get deeper into the
+ * hiddenGroupRefs, we'll increment the counter, call the group's unparser,
and as
+ * we unwind from the refs, we'll decrement.
+ */
+class HiddenGroupCombinatorUnparser(
+ ctxt: ModelGroupRuntimeData, bodyUnparser: Unparser)
+ extends CombinatorUnparser(ctxt) {
+
+ override lazy val childProcessors = Vector(bodyUnparser)
+
+ override lazy val runtimeDependencies = Vector()
+
+ def unparse(start: UState): Unit = {
+start.incrementHiddenDef
+
+// unparse
+bodyUnparser.unparse1(start)
+
+start.decrementHiddenDef
+ }
Review comment:
We probably want to wrap this in a a try/finally so that the hiddenef count
always decremented even if the bodyUnparse throws some exception.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389899042
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/HiddenGroupCombinatorParser.scala
##
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.processors.parsers
+
+import org.apache.daffodil.processors.ModelGroupRuntimeData
+
+/**
+ * The purpose of this combinator is to increment/decrement the depth counter
+ * for hidden groups, call the group's parser. As we get deeper into the
+ * hiddenGroupRefs, we'll increment the counter, call the group's parser, and
as
+ * we unwind from the refs, we'll decrement.
+ */
+class HiddenGroupCombinatorParser(
+ ctxt: ModelGroupRuntimeData, bodyParser: Parser)
+ extends CombinatorParser(ctxt) {
+
+ override lazy val childProcessors = Vector(bodyParser)
+
+ override lazy val runtimeDependencies = Vector()
+
+ def parse(start: PState): Unit = {
+start.incrementHiddenDef
+
+// parse
+bodyParser.parse1(start)
+
+start.decrementHiddenDef
Review comment:
Same here, probably need a try/finally.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389896414
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/HiddenGroupCombinator.scala
##
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.grammar.primitives
+
+import org.apache.daffodil.dsom.ModelGroup
+import org.apache.daffodil.grammar.Gram
+import org.apache.daffodil.grammar.Terminal
+import org.apache.daffodil.processors.parsers.Parser
+import org.apache.daffodil.processors.unparsers.Unparser
+import org.apache.daffodil.processors.parsers.HiddenGroupCombinatorParser
+import org.apache.daffodil.processors.unparsers.HiddenGroupCombinatorUnparser
+
+final class HiddenGroupCombinator(
+ctxt: ModelGroup,
+body: Gram
+) extends Terminal(ctxt, !body.isEmpty){
+
+ lazy val parser: Parser = new HiddenGroupCombinatorParser(
ctxt.modelGroupRuntimeData, body.parser)
+
+ override lazy val unparser: Unparser = new HiddenGroupCombinatorUnparser(
ctxt.modelGroupRuntimeData, body.unparser)
+
+}
Review comment:
Check spaces here. There should be two-space indentation and there's some
extra spaces.
Also, the parenthesis closing the HiddenGroupCombinator should be on the
same line as the last arguement. Eg.
```scala
final class HiddenGroupCombinator(
ctxt: ModelGroup,
body: Gram)
extends Terminal(ctxt, !body.isEmpty){
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389892751
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ModelGroupGrammarMixin.scala
##
@@ -68,9 +69,16 @@ trait ModelGroupGrammarMixin
case c: ChoiceTermBase => DelimiterStackCombinatorChoice(c, content)
case s: SequenceTermBase => DelimiterStackCombinatorSequence(s,
content)
}
- } else { groupContentDef }
+ } else {
+groupContent
+ } //TODO: confirm this works; changed from groupContentDef
-finalContent
+ val mgrd = self.modelGroupRuntimeData
+ if (mgrd.isHidden) {
Review comment:
Does isHidden need to be part of the MoelGroupRuntimeData? Can we just use
isHidden frome the ModelGroup DSOM?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #332: Refactor isHidden
stevedlawrence commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r389896732
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/ChoiceTermRuntime1Mixin.scala
##
@@ -140,6 +140,7 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
optIgnoreCase,
maybeFillByteEv,
maybeCheckByteAndBitOrderEv,
- maybeCheckBitOrderAndCharset)
+ maybeCheckBitOrderAndCharset,
+ isHidden)
Review comment:
Is this needed? Seems all hidden logic is either done at compile time or
using the new hidden stack thing.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] olabusayoT opened a new pull request #332: Refactor isHidden
olabusayoT opened a new pull request #332: Refactor isHidden URL: https://github.com/apache/incubator-daffodil/pull/332 - Removes isHidden from ElementRuntimeData - Adds isHidden flag to ModelGroup/ModelGroupRuntimeData - Adds private hiddenDepth counter (and associated functions) increment/decrement logic (HiddenGroupCombinators) for PState/UState - overrides increment/decrement functions in UStateForSuspension to fail on UsageError as these have no place in that context - Adds check to verify we are out of hidden state at the end of parsing/unparsing - Adds isHidden check to ChoiceGroupRef - Adds isHidden/setHidden logic to InfosetElement/DIElement - Adds tests for group defs used in both hidden/non-hidden sequence & choice contexts -- adds "info hidden" to debugger with tests -- moves InfoOccursIndex to maintain alphabetic ordering of objects DAFFODIL-2281 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: us...@infra.apache.org With regards, Apache Git Services
[jira] [Closed] (DAFFODIL-2283) ATO and HL7 saved parsers fail with Stack Overflow errors during generation on commit 7e8e1b3e
[ https://issues.apache.org/jira/browse/DAFFODIL-2283?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Dave Thompson closed DAFFODIL-2283. --- Verified the specified commit(commit 126789891ed5e834037198e5aa59e1f6b34dcb04) is included in the latest pull from the incubator_daffodil repo. Verified the affected schemas compile and saved parsers successfully. Verified the nightly test for the specified affected formats successfully complete. > ATO and HL7 saved parsers fail with Stack Overflow errors during generation > on commit 7e8e1b3e > -- > > Key: DAFFODIL-2283 > URL: https://issues.apache.org/jira/browse/DAFFODIL-2283 > Project: Daffodil > Issue Type: Bug > Components: DFDL Schemas >Affects Versions: 2.5.0 > Environment: Nightly test platform. >Reporter: Dave Thompson >Assignee: Steve Lawrence >Priority: Blocker > Fix For: 2.6.0 > > Attachments: ATO_SavedParserFail.txt, HL7_SavedParserFail.txt > > > The ATO and HL7 saved parser (.bin) files fail to be generated on commit > 7e8e1b3ed110efe939908cdf1e12df50d1f86527, commit for schema compilation speed > improvement. > Both formats fail with a Stack Overflow error. > Attached are the trace back message for each. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Resolved] (DAFFODIL-2283) ATO and HL7 saved parsers fail with Stack Overflow errors during generation on commit 7e8e1b3e
[ https://issues.apache.org/jira/browse/DAFFODIL-2283?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Steve Lawrence resolved DAFFODIL-2283. -- Resolution: Fixed Fixed in commit 126789891ed5e834037198e5aa59e1f6b34dcb04 > ATO and HL7 saved parsers fail with Stack Overflow errors during generation > on commit 7e8e1b3e > -- > > Key: DAFFODIL-2283 > URL: https://issues.apache.org/jira/browse/DAFFODIL-2283 > Project: Daffodil > Issue Type: Bug > Components: DFDL Schemas >Affects Versions: 2.5.0 > Environment: Nightly test platform. >Reporter: Dave Thompson >Assignee: Steve Lawrence >Priority: Blocker > Fix For: 2.6.0 > > Attachments: ATO_SavedParserFail.txt, HL7_SavedParserFail.txt > > > The ATO and HL7 saved parser (.bin) files fail to be generated on commit > 7e8e1b3ed110efe939908cdf1e12df50d1f86527, commit for schema compilation speed > improvement. > Both formats fail with a Stack Overflow error. > Attached are the trace back message for each. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[incubator-daffodil] branch master updated: Fix deep stack sizes when serializing some schemas
This is an automated email from the ASF dual-hosted git repository.
slawrence pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-daffodil.git
The following commit(s) were added to refs/heads/master by this push:
new 1267898 Fix deep stack sizes when serializing some schemas
1267898 is described below
commit 126789891ed5e834037198e5aa59e1f6b34dcb04
Author: Steve Lawrence
AuthorDate: Mon Mar 9 09:19:46 2020 -0400
Fix deep stack sizes when serializing some schemas
The "parents" val in a DPathCompileInfo is a backpointer to all
DPathCompileInfo's that reference it. The problem with this is that when
elements are shared, these backpointers create a highly connected graph
that requires a large stack to serialize using the default java
serialization as it jumps around parents and children. To avoid this
large stack requirement, we make the parents backpointer transient. This
prevents jumping back up to parents during serialization and results in
only needing a stack depth relative to the schema depth. Once all that
serialization is completed and all the DPathCompileInfo's are
serialized, we then manually traverse all the DPathCompileInfo's again
and serialize the parent sequences (via the serailizeParents method).
Because all the DPathCompileInfo's are already serialized, this just
serializes the Sequence objects and the stack depth is again relative to
the schema depth.
On complex schemas, this saw an order of magnitude reduction in stack
size during serialization.
DAFFODIL-2283
---
.../scala/org/apache/daffodil/util/Serialize.scala | 1 -
.../apache/daffodil/dsom/CompiledExpression1.scala | 54 +-
.../daffodil/processors/SchemaSetRuntimeData.scala | 10
3 files changed, 62 insertions(+), 3 deletions(-)
diff --git
a/daffodil-lib/src/main/scala/org/apache/daffodil/util/Serialize.scala
b/daffodil-lib/src/main/scala/org/apache/daffodil/util/Serialize.scala
index 1a94fa8..20bb92e 100644
--- a/daffodil-lib/src/main/scala/org/apache/daffodil/util/Serialize.scala
+++ b/daffodil-lib/src/main/scala/org/apache/daffodil/util/Serialize.scala
@@ -41,7 +41,6 @@ trait PreSerialization extends Serializable {
protected final def serializeObject(out: java.io.ObjectOutputStream) {
try {
- // println("serializing " + Misc.getNameFromClass(this)) // good place
for a breakpoint
preSerialization
out.defaultWriteObject()
} catch {
diff --git
a/daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
b/daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
index da9c5a4..6a6c985 100644
---
a/daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
+++
b/daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
@@ -212,7 +212,47 @@ class DPathCompileInfo(
extends ImplementsThrowsSDE with PreSerialization
with HasSchemaFileLocation {
- lazy val parents = parentsArg
+
+ /**
+ * This "parents" val is a backpointer to all DPathCompileInfo's that
+ * reference this DPathCompileInfo. The problem with this is that when
+ * elements are shared, these backpointers create a highly connected graph
+ * that requires a large stack to serialize using the default java
+ * serialization as it jumps around parents and children. To avoid this large
+ * stack requirement, we make the parents backpointer transient. This
+ * prevents jumping back up to parents during serialization and results in
+ * only needing a stack depth relative to the schema depth. Once all that
+ * serialization is completed and all the DPathCompileInfo's are serialized,
+ * we then manually traverse all the DPathCompileInfo's again and serialize
+ * the parent sequences (via the serailizeParents method). Because all the
+ * DPathCompileInfo's are already serialized, this just serializes the
+ * Sequence objects and the stack depth is again relative to the schema
+ * depth.
+ */
+ @transient
+ val parents = parentsArg
+
+ def serializeParents(oos: java.io.ObjectOutputStream): Unit = {
+oos.writeObject(parents)
+ }
+
+ def deserializeParents(ois: java.io.ObjectInputStream): Unit = {
+val deserializedParents =
ois.readObject().asInstanceOf[Seq[DPathCompileInfo]]
+
+// Set the parents field via reflection so that it can be a val rather
than a var
+val clazz = this.getClass
+val parentsField = try {
+ clazz.getDeclaredField("parents")
+} catch {
+ case e: java.lang.NoSuchFieldException =>
+clazz.getSuperclass.getDeclaredField("parents")
+}
+parentsField.setAccessible(true)
+parentsField.set(this, deserializedParents) // set the value to the
deserialized value
+parentsField.setAccessible(false)
+ }
+
+
lazy val variableMap =
variableMapArg
@@ -222,7 +262,6 @@
[GitHub] [incubator-daffodil] stevedlawrence merged pull request #331: Fix deep stack sizes when serializing some schemas
stevedlawrence merged pull request #331: Fix deep stack sizes when serializing some schemas URL: https://github.com/apache/incubator-daffodil/pull/331 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #331: Fix deep stack sizes when serializing some schemas
stevedlawrence commented on a change in pull request #331: Fix deep stack sizes
when serializing some schemas
URL: https://github.com/apache/incubator-daffodil/pull/331#discussion_r389783752
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
##
@@ -212,7 +212,48 @@ class DPathCompileInfo(
extends ImplementsThrowsSDE with PreSerialization
with HasSchemaFileLocation {
+
+ /**
+ * This "parents" val is a backpointer to all DPathCompileInfo's that
+ * reference this DPathCompileInfo. The problem with this is that when
+ * elements are shared, these backpointers create a highly connected graph
+ * that requires a large stack to serialize using the default java
+ * serialization as it jumps around parents and children. To avoid this large
+ * stack requirement, we make the parents backpointer transient. This
+ * prevents jumping back up to parents during serialization and results in
+ * only needing a stack depth relative to the schema depth. Once all that
+ * serialization is completed and all the DPathCompileInfo's are serialized,
+ * we then manually traverse all the DPathCompileInfo's again and serialize
+ * the parent sequences (via the serailizeParents method). Because all the
+ * DPathCompileInfo's are already serialized, this just serializes the
+ * Sequence objects and the stack depth is again relative to the schema
+ * depth.
+ */
+ @transient
lazy val parents = parentsArg
Review comment:
Looks like val's in a constructor parameter cannot be made transient.
Keeping the parentsArg and using a lazy val I think is the normal solution to
this kindof thing.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #331: Fix deep stack sizes when serializing some schemas
stevedlawrence commented on a change in pull request #331: Fix deep stack sizes
when serializing some schemas
URL: https://github.com/apache/incubator-daffodil/pull/331#discussion_r389775675
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
##
@@ -212,7 +212,48 @@ class DPathCompileInfo(
extends ImplementsThrowsSDE with PreSerialization
with HasSchemaFileLocation {
+
+ /**
+ * This "parents" val is a backpointer to all DPathCompileInfo's that
+ * reference this DPathCompileInfo. The problem with this is that when
+ * elements are shared, these backpointers create a highly connected graph
+ * that requires a large stack to serialize using the default java
+ * serialization as it jumps around parents and children. To avoid this large
+ * stack requirement, we make the parents backpointer transient. This
+ * prevents jumping back up to parents during serialization and results in
+ * only needing a stack depth relative to the schema depth. Once all that
+ * serialization is completed and all the DPathCompileInfo's are serialized,
+ * we then manually traverse all the DPathCompileInfo's again and serialize
+ * the parent sequences (via the serailizeParents method). Because all the
+ * DPathCompileInfo's are already serialized, this just serializes the
+ * Sequence objects and the stack depth is again relative to the schema
+ * depth.
+ */
+ @transient
lazy val parents = parentsArg
Review comment:
Note that there was some issue with ``@TransientParam`` not working as
expected if I just renamed the parameter to ``parents`` and made it a ``val``.
Change this from ``lazy val`` to ``val`` does work as expected though.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #331: Fix deep stack sizes when serializing some schemas
stevedlawrence commented on a change in pull request #331: Fix deep stack sizes
when serializing some schemas
URL: https://github.com/apache/incubator-daffodil/pull/331#discussion_r389774383
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
##
@@ -212,7 +212,48 @@ class DPathCompileInfo(
extends ImplementsThrowsSDE with PreSerialization
with HasSchemaFileLocation {
+
+ /**
+ * This "parents" val is a backpointer to all DPathCompileInfo's that
+ * reference this DPathCompileInfo. The problem with this is that when
+ * elements are shared, these backpointers create a highly connected graph
+ * that requires a large stack to serialize using the default java
+ * serialization as it jumps around parents and children. To avoid this large
+ * stack requirement, we make the parents backpointer transient. This
+ * prevents jumping back up to parents during serialization and results in
+ * only needing a stack depth relative to the schema depth. Once all that
+ * serialization is completed and all the DPathCompileInfo's are serialized,
+ * we then manually traverse all the DPathCompileInfo's again and serialize
+ * the parent sequences (via the serailizeParents method). Because all the
+ * DPathCompileInfo's are already serialized, this just serializes the
+ * Sequence objects and the stack depth is again relative to the schema
+ * depth.
+ */
+ @transient
lazy val parents = parentsArg
Review comment:
Correct. Will fix.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #331: Fix deep stack sizes when serializing some schemas
stevedlawrence commented on a change in pull request #331: Fix deep stack sizes
when serializing some schemas
URL: https://github.com/apache/incubator-daffodil/pull/331#discussion_r389774182
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
##
@@ -301,6 +341,19 @@ class DPathElementCompileInfo(
unqualifiedPathStepPolicy,
typeCalcMap, lexicalContextRuntimeData) {
+ override
Review comment:
Yeah,not sure what I was thinking here. Fixed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #331: Fix deep stack sizes when serializing some schemas
mbeckerle commented on a change in pull request #331: Fix deep stack sizes when
serializing some schemas
URL: https://github.com/apache/incubator-daffodil/pull/331#discussion_r389764048
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
##
@@ -212,7 +212,48 @@ class DPathCompileInfo(
extends ImplementsThrowsSDE with PreSerialization
with HasSchemaFileLocation {
+
+ /**
+ * This "parents" val is a backpointer to all DPathCompileInfo's that
+ * reference this DPathCompileInfo. The problem with this is that when
+ * elements are shared, these backpointers create a highly connected graph
+ * that requires a large stack to serialize using the default java
+ * serialization as it jumps around parents and children. To avoid this large
+ * stack requirement, we make the parents backpointer transient. This
+ * prevents jumping back up to parents during serialization and results in
+ * only needing a stack depth relative to the schema depth. Once all that
+ * serialization is completed and all the DPathCompileInfo's are serialized,
+ * we then manually traverse all the DPathCompileInfo's again and serialize
+ * the parent sequences (via the serailizeParents method). Because all the
+ * DPathCompileInfo's are already serialized, this just serializes the
+ * Sequence objects and the stack depth is again relative to the schema
+ * depth.
+ */
+ @transient
lazy val parents = parentsArg
Review comment:
lazy val no longer needed as parentsArg is not a by-name argument. Could
just rename the arg to parents.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #331: Fix deep stack sizes when serializing some schemas
mbeckerle commented on a change in pull request #331: Fix deep stack sizes when
serializing some schemas
URL: https://github.com/apache/incubator-daffodil/pull/331#discussion_r389744380
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
##
@@ -301,6 +341,19 @@ class DPathElementCompileInfo(
unqualifiedPathStepPolicy,
typeCalcMap, lexicalContextRuntimeData) {
+ override
Review comment:
This formatting with the override keyword on a separate line is unusual and
non-standard for us.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence opened a new pull request #331: Fix deep stack sizes when serializing some schemas
stevedlawrence opened a new pull request #331: Fix deep stack sizes when serializing some schemas URL: https://github.com/apache/incubator-daffodil/pull/331 The "parents" val in a DPathCompileInfo is a backpointer to all DPathCompileInfo's that reference it. The problem with this is that when elements are shared, these backpointers create a highly connected graph that requires a large stack to serialize using the default java serialization as it jumps around parents and children. To avoid this large stack requirement, we make the parents backpointer transient. This prevents jumping back up to parents during serialization and results in only needing a stack depth relative to the schema depth. Once all that serialization is completed and all the DPathCompileInfo's are serialized, we then manually traverse all the DPathCompileInfo's again and serialize the parent sequences (via the serailizeParents method). Because all the DPathCompileInfo's are already serialized, this just serializes the Sequence objects and the stack depth is again relative to the schema depth. On complex schemas, this saw an order of magnitude reduction in stack size during serialization. DAFFODIL-2283 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: us...@infra.apache.org With regards, Apache Git Services
[jira] [Assigned] (DAFFODIL-2283) ATO and HL7 saved parsers fail with Stack Overflow errors during generation on commit 7e8e1b3e
[ https://issues.apache.org/jira/browse/DAFFODIL-2283?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Steve Lawrence reassigned DAFFODIL-2283: Assignee: Steve Lawrence (was: Mike Beckerle) > ATO and HL7 saved parsers fail with Stack Overflow errors during generation > on commit 7e8e1b3e > -- > > Key: DAFFODIL-2283 > URL: https://issues.apache.org/jira/browse/DAFFODIL-2283 > Project: Daffodil > Issue Type: Bug > Components: DFDL Schemas >Affects Versions: 2.5.0 > Environment: Nightly test platform. >Reporter: Dave Thompson >Assignee: Steve Lawrence >Priority: Blocker > Fix For: 2.6.0 > > Attachments: ATO_SavedParserFail.txt, HL7_SavedParserFail.txt > > > The ATO and HL7 saved parser (.bin) files fail to be generated on commit > 7e8e1b3ed110efe939908cdf1e12df50d1f86527, commit for schema compilation speed > improvement. > Both formats fail with a Stack Overflow error. > Attached are the trace back message for each. -- This message was sent by Atlassian Jira (v8.3.4#803005)
