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

less allocation in ExecutionPath.add #769

Merged
merged 5 commits into from
Oct 28, 2021

Conversation

yanns
Copy link
Contributor

@yanns yanns commented Oct 28, 2021

One way to fix #768
The issue with that approach is the usage of reverse that builds a new List.

@yanns
Copy link
Contributor Author

yanns commented Oct 28, 2021

@agourlay WDYT?

@@ -4,33 +4,35 @@ import sangria.marshalling.ResultMarshaller
import sangria.ast
import sangria.schema.ObjectType

case class ExecutionPath private (path: Vector[Any], cacheKeyPath: ExecutionPath.PathCacheKey) {
class ExecutionPath private (_path: List[Any], cacheKeyPath: ExecutionPath.PathCacheKey) {
lazy val path: List[Any] = _path.reverse
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could expose a Vector here to have less breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we never use the indexing aspect of the Vector, I vote for getting rid of it if we can have breaking changes.

case s: String => m.scalarNode(s, "String", Set.empty)
case i: Int => m.scalarNode(i, "Int", Set.empty)
})
}.toVector)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's faster to reverse a Vector than a List.

@yanns
Copy link
Contributor Author

yanns commented Oct 28, 2021

To avoid the reverse, we could use a double linked list, but there's only a mutable one in scala: https://www.scala-lang.org/api/2.12.4/scala/collection/mutable/DoubleLinkedList.html

@yanns
Copy link
Contributor Author

yanns commented Oct 28, 2021

a mutable List could be a better choice

In the immutable world, cats Chain would be a good fit, but we should not add cats as a dependency.

@agourlay
Copy link
Contributor

agourlay commented Oct 28, 2021

Thanks a lot for tackling this issue 👍

a mutable List could be a better choice

what is the thread safety story here for this class?

In the immutable world...cats

yes the cats dependency is a no go I think.

To avoid the reverse...

in both cases you could use reverseIterator I believe

@agourlay
Copy link
Contributor

Also a Queue could be a possible candidate as it is built-in on two internal lists.


/** @return
* last index in the path, if available
*/
def lastIndex: Option[Int] = path.lastOption.collect { case i: Int => i }
def lastIndex: Option[Int] = _path.headOption.collect { case i: Int => i }

/** @return
* the size of the path excluding the indexes
*/
def size = cacheKeyPath.size / 2
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally the cacheKeyPath.size should be cached as it is O(n) on a List

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yanns
Copy link
Contributor Author

yanns commented Oct 28, 2021

Also a Queue could be a possible candidate as it is built-in on two internal lists.

Not sure about the performance when iterating on all elements.
For example, iterator performs a reverse.

override def iterator: Iterator[A] = out.iterator.concat(in.reverse)

and we would need to use appended to have the correct order, adding elements to in.

@yanns
Copy link
Contributor Author

yanns commented Oct 28, 2021

a mutable List could be a better choice

what is the thread safety story here for this class?

I don't think we have concurrency update / access on it.

To avoid the reverse...

in both cases you could use reverseIterator I believe

👍
4f4f529

Copy link
Contributor

@agourlay agourlay left a comment

Choose a reason for hiding this comment

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

This looks like a clear improvement to me!

🐑

@yanns yanns merged commit e3c2731 into main Oct 28, 2021
@yanns yanns deleted the issue_768_allocation_in_ExecutionPath_add branch October 28, 2021 11:50
yanns added a commit that referenced this pull request Oct 29, 2021
Follow-up on #769
avoid breaking types of `path` and `cacheKey`.

Fix #773
yanns added a commit that referenced this pull request Oct 29, 2021
Follow-up on #769
avoid breaking types of `path` and `cacheKey`.

Fix #773
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExecutionPath.add allocates a lot of Vectors
2 participants