Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid extra array creation when creating a list of a single item #1935

Merged
merged 1 commit into from
Jan 27, 2018

Conversation

chriswebster
Copy link

Mailing List thread:
https://groups.google.com/forum/#!topic/liftweb/jD3OG9jD_hs

The list creation in findDirectByName was using variable argument constructor which was performing unnecessary array allocations as part of the read case class flow. This change creates the single item list directly bypassing the extra array allocation and optimizing performance.

The benchmark results were as follows:

before change
3.2.0-RC1
[info] LiftJsonBenchmark.readCaseClass thrpt 10 443843.164 ± 5996.863 ops/s

after change
[info] LiftJsonBenchmark.readCaseClass thrpt 10 584527.678 ± 11315.888 ops/s

Copy link
Member

@farmdawgnation farmdawgnation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things look good. Will give others a chance to look at it before merging.

Thanks for the contribution!

@@ -118,7 +118,7 @@ object JsonAST {
* nothing is found, you'll get a `JNothing`.
*/
def \(nameToFind: String): JValue = {
findDirectByName(List(this), nameToFind) match {
findDirectByName(this :: Nil, nameToFind) match {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm debating about whether or not this is an obscure enough change to document somehow. I'll keep thinking on it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth leaving a note here, in the style of “Use :: instead of List() to avoid an extra array allocation.”

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@farmdawgnation farmdawgnation mentioned this pull request Jan 19, 2018
@farmdawgnation farmdawgnation merged commit 62b6dd3 into lift:master Jan 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants