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/01/23 19:49:34 UTC
[GitHub] [incubator-daffodil] stevedlawrence opened a new pull request #316:
Assortment of changes to improve performance
stevedlawrence opened a new pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316
- Optimize how we parse date/times from an XML infoset. Previously we
attempted to parse a handful of different potential patterns using
ICU4J SimpleDateFormat.parse(). Profiling show this simple parse
performed many allocations. Considering that this is a fairly strict
format we can implement the parsing ourselves, which is much more
efficient, minimizes allocations, and avoids thread-locals since
SimpleDateFormat is not thread safe.
- Instead of creating new calendar every time we parse an XML Date/Time,
clone an already existing one. The process to initialize a new
Calendar is pretty expensive, cloning is much more efficient.
- Skip remapping XML strings if they do not contain any invalid
characters. This avoids unnecessary string allocations in the common
case, but does incur some overhead in the rare case when remapping is
needed.
- Use the same empty Array of DFADelimiters in the delimiter stack.
Otherwise we allocate a bunch of empty arrays, and scala even ends up
allocating even ore stuff to support it (e.g. ClassTags).
- Fix MaybeBoolean to avoid allocating new MaybeBoolean's
- Use Array.length == 0 instead of Array.isEmpty in inner loops. Using
.isEmpty will allocate a scala ArrayOps, which is unnecssary if all
were doing is checking the length. Note that .size will also allocate
an ArrayOps.
- Fix maybeConstant_ in Evaluatable to avoid Maybe allocations
These issues were discovered while profiling an unparse of a CSV schema.
Performance averaged around 20% faster on my testing.
DAFFODIL-2222, DAFFODIL-1883
----------------------------------------------------------------
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] bsloane1650 commented on a change in pull
request #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370705004
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
Review comment:
The insight is that T and AnyRef are the same thing, so it should be possible to cast from Maybe[AnyRef] to Maybe[T] without overhead.
In DataValue.unsafeFromMaybeAnyRef, there is added complexity since the actual function is:
```
null -> DataValue.empty
value -> value
```
In the case of Maybe, we have:
```
null -> null
value -> value
```
which can be treated as a single case.
The analogous situation in DataValues, would be getNullablePrimitive:
``` @inline def getNullablePrimitive:DataValue.DataValuePrimitiveNullable = new DataValue(v) ```
which does the cast without doing anything.
Having said that, we are only talking about 1 conditional statement, so it probably isn't worth spending too much time on this.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370618719
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
+ // We could just do constValue_.asInstanceOf[Maybe[T]], but the compiled
+ // bytecode ends up allocating a Maybe. Since constant values are accessed
+ // very often, we really want to avoid that overhead. The following gets
+ // rid of those unnecessary allocations.
+ if (constValue_.isDefined) {
+ Maybe[T](constValue_.get.asInstanceOf[T])
Review comment:
Disably turned up about 200 places where we allocate a Maybe. From the looks of it, it looks like it's in places where boxing is required, or might be difficult to remove. I'd reccommend we only work to remove these when they show up in profiling.
----------------------------------------------------------------
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] bsloane1650 commented on a change in pull
request #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370361355
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
Review comment:
In theory, this should just be maybeConstant = constValue_. All we are trying to do here is a cast from T <: AnyRef : Maybe[AnyRef] -> Maybe[T], which is a complete no-op at runtime.
This feels like we should be using DataValuePrimitiveNullable instead of either Maybe type. However, I believe I decided that doing so would involve refactoring all of out expression classes that used T <: AnyRef, which we probably don't want to do.
We can however use the same trick in the Maybe class that I used in the DataValue class to do this conversion with 0 runtime overhead.
Essentially, as part of the Maybe class, define an inline function along the lines of: unsafeCast[T]:Maybe[T] = (Maybe(this.get))
This worked in the case of DataValues because T was purely a phantom type. In the case of Maybe, I think it should be possible to do something similar because of type erasure.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370633191
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
+ // We could just do constValue_.asInstanceOf[Maybe[T]], but the compiled
+ // bytecode ends up allocating a Maybe. Since constant values are accessed
+ // very often, we really want to avoid that overhead. The following gets
+ // rid of those unnecessary allocations.
+ if (constValue_.isDefined) {
+ Maybe[T](constValue_.get.asInstanceOf[T])
Review comment:
Will do. For reference, this was the command:
```bash
find daffodil.git -name '*.class' -exec javap -p -c '{}' \; > dissasembled.txt
```
From there, it's just a matter of grep, e.g.
```bash
grep -a new disassembled.txt | grep Maybe
```
Note that the -a flag is needed since there's UTF-8 characters or something that makes grep think the file is binary. Javap is really useful in figuring out why there are allocations since Scala does so much stuff behind your back.
----------------------------------------------------------------
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 #316:
Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370334358
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/DelimiterUnparsers.scala
##########
@@ -56,7 +56,7 @@ class DelimiterTextUnparser(override val context: TermRuntimeData, delimiterType
else localDelimNode.terminator
}
- if (delimDFAs.isEmpty) Assert.invariantFailed("Expected a delimiter of type " + delimiterType + " on the stack, but was not found.")
+ if (delimDFAs.length == 0) Assert.invariantFailed("Expected a delimiter of type " + delimiterType + " on the stack, but was not found.")
Review comment:
Is this stylistic, or is there actually an advantage to using length vs. 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull
request #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r371990965
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/DelimiterUnparsers.scala
##########
@@ -56,7 +56,7 @@ class DelimiterTextUnparser(override val context: TermRuntimeData, delimiterType
else localDelimNode.terminator
}
- if (delimDFAs.isEmpty) Assert.invariantFailed("Expected a delimiter of type " + delimiterType + " on the stack, but was not found.")
+ if (delimDFAs.length == 0) Assert.invariantFailed("Expected a delimiter of type " + delimiterType + " on the stack, but was not found.")
Review comment:
Tips I found are now on the wiki on our [Coding for Performance](https://cwiki.apache.org/confluence/display/DAFFODIL/Coding+for+Performance) page.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370618042
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetImpl.scala
##########
@@ -903,7 +903,7 @@ sealed trait DIElement
*/
final def maybeIsNilled: MaybeBoolean = {
if (!_isNilledSet) MaybeBoolean.Nope
- MaybeBoolean(_isNilled)
+ else MaybeBoolean(_isNilled)
Review comment:
After some investigation, while this code is logically wrong, we always use ``maybeIsNilled`` in a manner that doesn't cause any bugs. Previously, this function could only return two values: ``MaybeBoolean.True`` or ``MaybeBoolean.false``. If nilled hadn't been set yet, it would return ``MaybeBoolean.False`` since false is the default value of _isNilled.
Whenever ``maybeIsNilled`` is used, we always have something to the affect of:
```
if (maybeIsNilled.isDefined && maybeIsNilled.get == true)
```
The logic is the same regardless if we return MaybeBoolean.Nope or MaybeBoolean.False. The only place this could cause a bug is we actually cared if maybeIsNilled hadn't been set yet, or or it had be explicitly set to false. We don't ever do that, so there isn't currently a bug.
That said, I'm wondering if we should even have maybeIsNilled. The above conditional seems wrong to me. If maybeIsNilled hasn't been set yet, we shouldn't be assuming it's false. In fact, I can't imagine a case where want to know if something is nillled but don't have an answer yet. Seems like this should be able to go away and we should just always be able to ask isNilled and get an answer.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370628288
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetImpl.scala
##########
@@ -903,7 +903,7 @@ sealed trait DIElement
*/
final def maybeIsNilled: MaybeBoolean = {
if (!_isNilledSet) MaybeBoolean.Nope
- MaybeBoolean(_isNilled)
+ else MaybeBoolean(_isNilled)
Review comment:
Sounds reasonable to me.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370363391
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetImpl.scala
##########
@@ -903,7 +903,7 @@ sealed trait DIElement
*/
final def maybeIsNilled: MaybeBoolean = {
if (!_isNilledSet) MaybeBoolean.Nope
- MaybeBoolean(_isNilled)
+ else MaybeBoolean(_isNilled)
Review comment:
Yeah, this definitely feels like a bug, but code coverage does show both lines as being hit. I'll see if I can figure out when this is used and see if there's a way to reproduce whatever bug already existed.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370360955
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/DelimiterUnparsers.scala
##########
@@ -56,7 +56,7 @@ class DelimiterTextUnparser(override val context: TermRuntimeData, delimiterType
else localDelimNode.terminator
}
- if (delimDFAs.isEmpty) Assert.invariantFailed("Expected a delimiter of type " + delimiterType + " on the stack, but was not found.")
+ if (delimDFAs.length == 0) Assert.invariantFailed("Expected a delimiter of type " + delimiterType + " on the stack, but was not found.")
Review comment:
Yep, there's an actual advantage here. delimDFAs is type ``Array[T]``. Scala will normally do the right than and represent this in byte code as a java array[]. All is good. However, if you try to use any functions that are normally Scala Sequence-y type things (e.g. isEmpty, .size), Scala will implicitly wrap the array in an ``ArrayOps``. This gives you all the nice scala-y stuff that were used to. However, this means we are allocating an ArrayOps just do figure out if the array is empty or not. While it's convenient and looks nice, it added does add allocations. I'm not sure if it really affected the performance, but this was called a ton, and made the profiling very noise, so this at least helps to get rid of some of that noise.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370626372
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetImpl.scala
##########
@@ -903,7 +903,7 @@ sealed trait DIElement
*/
final def maybeIsNilled: MaybeBoolean = {
if (!_isNilledSet) MaybeBoolean.Nope
- MaybeBoolean(_isNilled)
+ else MaybeBoolean(_isNilled)
Review comment:
So this distinction of MaybeBoolean.Nope vs. true/false I can see it being needed for implementing unparser default values, or possibly even parser default values.
It may have been put in, in anticipation of those features.
Also the expression language currently has no way to provide a "nil" value for a nillable element. This is likely a shortcoming.
Were that to be added, we would need exactly this mechanism to detect completion of unparser expression evaluation.
I think we should leave this functionality in place for now. Albeit, it was broken.
----------------------------------------------------------------
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] bsloane1650 commented on a change in pull
request #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370350257
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
+ // We could just do constValue_.asInstanceOf[Maybe[T]], but the compiled
+ // bytecode ends up allocating a Maybe. Since constant values are accessed
+ // very often, we really want to avoid that overhead. The following gets
+ // rid of those unnecessary allocations.
+ if (constValue_.isDefined) {
+ Maybe[T](constValue_.get.asInstanceOf[T])
Review comment:
When I was working on the DataValues class, I got into a workflow, where I would look dissassembly all the *.class files and look for allocations of DataValue. I also did a similar trick when I was looking for scala.lang.BigInt/BigDecimal.
It is probably more work then it is worth to do this in an automated fashion (particularly because removing all boxing would likely be a lot more effort then it is worth), however, it might be worth doing this as a one off. Possibly also document the process on the wiki and repeat it manually on occasion.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370624018
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/DelimiterUnparsers.scala
##########
@@ -56,7 +56,7 @@ class DelimiterTextUnparser(override val context: TermRuntimeData, delimiterType
else localDelimNode.terminator
}
- if (delimDFAs.isEmpty) Assert.invariantFailed("Expected a delimiter of type " + delimiterType + " on the stack, but was not found.")
+ if (delimDFAs.length == 0) Assert.invariantFailed("Expected a delimiter of type " + delimiterType + " on the stack, but was not found.")
Review comment:
Deserves a note in our performance page on the wiki saying when to prefer java arrays to sequences, and then ifyou are using java arrays, how to use them so as to avoid the downsides of this particular flavor of auto-conversion.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370365114
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
+ // We could just do constValue_.asInstanceOf[Maybe[T]], but the compiled
+ // bytecode ends up allocating a Maybe. Since constant values are accessed
+ // very often, we really want to avoid that overhead. The following gets
+ // rid of those unnecessary allocations.
+ if (constValue_.isDefined) {
+ Maybe[T](constValue_.get.asInstanceOf[T])
Review comment:
There are no other places we do ``asInstanceOf[Maybe[...]]``. I'll try disassemble things and see if there's anything that jumps out.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370734936
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
Review comment:
I've just pushed a change that I *think* is what you describe? Does that look right?
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370334843
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetImpl.scala
##########
@@ -903,7 +903,7 @@ sealed trait DIElement
*/
final def maybeIsNilled: MaybeBoolean = {
if (!_isNilledSet) MaybeBoolean.Nope
- MaybeBoolean(_isNilled)
+ else MaybeBoolean(_isNilled)
Review comment:
This looks to be a bug fix. We should consider adding a test that would have caught it, since there obviously wasn't one before.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370628494
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/DelimiterUnparsers.scala
##########
@@ -56,7 +56,7 @@ class DelimiterTextUnparser(override val context: TermRuntimeData, delimiterType
else localDelimNode.terminator
}
- if (delimDFAs.isEmpty) Assert.invariantFailed("Expected a delimiter of type " + delimiterType + " on the stack, but was not found.")
+ if (delimDFAs.length == 0) Assert.invariantFailed("Expected a delimiter of type " + delimiterType + " on the stack, but was not found.")
Review comment:
Agreed, I'll add a write to our wiki page about all the tricks I'm finding to reduce allocations.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370336336
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
+ // We could just do constValue_.asInstanceOf[Maybe[T]], but the compiled
+ // bytecode ends up allocating a Maybe. Since constant values are accessed
+ // very often, we really want to avoid that overhead. The following gets
+ // rid of those unnecessary allocations.
+ if (constValue_.isDefined) {
+ Maybe[T](constValue_.get.asInstanceOf[T])
Review comment:
This is interesting. So yeah, if you return constValue_ and cast it, that's a generic operation, which requires a boxed object.
We should consider if this can be caught somehow. At minimum we can search for "asInstanceOf[Maybe" and see if any others come up.
----------------------------------------------------------------
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] bsloane1650 commented on a change in pull
request #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370743703
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
Review comment:
That looks correct.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370627620
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
Review comment:
Are you sure? ``maybeConst_`` is defined as a ``Maybe[AnyRef]``. Although we know it will always be a ``Maybe[T]``, it's a var and could theoretcially be changed to something other than a ``Maybe[T]``. Seems like the ``unsafeFromMaybeAnyRef`` is more like what we need, which is somewhat similar to the new code.
Re: DataValuePrimitiveNullable, Evaluatables can result in things that are not DataValues, so I'm not sure we could do that.
----------------------------------------------------------------
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 #316: Assortment of changes to improve performance
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #316: Assortment of changes to improve performance
URL: https://github.com/apache/incubator-daffodil/pull/316#discussion_r370629373
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/Evaluatable.scala
##########
@@ -285,7 +285,17 @@ abstract class Evaluatable[+T <: AnyRef](protected val ci: DPathCompileInfo, qNa
/**
* Preferred for use in the runtime.
*/
- @inline final def maybeConstant = constValue_.asInstanceOf[Maybe[T]]
+ @inline final def maybeConstant = {
+ // We could just do constValue_.asInstanceOf[Maybe[T]], but the compiled
+ // bytecode ends up allocating a Maybe. Since constant values are accessed
+ // very often, we really want to avoid that overhead. The following gets
+ // rid of those unnecessary allocations.
+ if (constValue_.isDefined) {
+ Maybe[T](constValue_.get.asInstanceOf[T])
Review comment:
I'm not exactly sure how to do this, so adding a note to this effect to the wiki page about performance coding is worth doing in a section (probably already is one) about avoiding excess boxing and unnecessary allocations.
----------------------------------------------------------------
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