THRIFT-4712: Improve Performance of ShortStack #1664
Conversation
|
Appreciate your changes. If you could also look at the backlog in Apache Jira and see if there are any existing issues you can tackle, that would also be appreciated. |
|
Sure thing. Thanks. |
| } | ||
|
|
||
| public void clear() { | ||
| top = -1; | ||
| top = 0; |
There was a problem hiding this comment.
Does this actually clear? I don't think it does. It resets top to 0 and the next peek() will read whatever was there before. Shouldn't clear actually clear? If one peek()s after clear and there's nothing there, what should it do? Perhaps throw?
There was a problem hiding this comment.
Good call. Well, the peek() method is not used anywhere other than the test framework, so it's a bit of a moot point; it should probably be removed altogether.
I think this is another example where the class should not be made public since it serves a very specific purpose. If it were scoped back into the package, it would have to be moved into the same package as the TCompactProtocol since that is what relies on this class. Thoughts?
There was a problem hiding this comment.
ShortStack was written in 2010. Perhaps it would be a good idea to analyze the performance of ShortStack managing 15 shorts vs. java.util.Stack in a more recent version of Java like 1.8. My guess is by now they've ironed out performance issues in containers like this... ideally we would try to remove ShortStack.java and its test all together.
There was a problem hiding this comment.
JDK is still slow because of the auto-boxing of the primitive short value. In most cases, it wouldn't make a difference, but for a serialization library that needs to be as fast as possible, it's measurable.
There was a problem hiding this comment.
Do you have evidence of this or is it just theory?
There was a problem hiding this comment.
Ya, I benchmarked it. I did two different scenarios. I didn't even bother including the Java JDK Stack implementation in the more aggressive scenario because it wasn't even close in the more trivial test.
belugabehr
force-pushed
the
THRIFT-4712
branch
from
6bb75f0
to
4d88939
Compare
|
The clear() method does the job. It sets the pointed back to the first open slot so that it can be overwritten. This is much faster than clearing the entire buffer or allocating a new one, and for these purposes, works well, now that I also removed the peek() method. |
No description provided.