Reduce size of buffer, stringBuffer and tape. - #42
Conversation
Sorry, something went wrong.
|
I believe in future, the JsonValue might be deep copied, thus the size of byte[] and long[] is important. |
Sorry, something went wrong.
In class JsonValue, the default size of buffer and stringBuffer, as well as long[] tape in class Tape, is 34M. But in practice it's not necessary. This patch reduce the size of them from 34M to its actual size.
| private byte[] padIfNeeded(byte[] buffer, int len) { | ||
| if (buffer.length - len < PADDING) { | ||
| if (buffer.length - len < PADDING && len < capacity) { | ||
| byte[] paddedBuffer = new byte[len + PADDING]; |
There was a problem hiding this comment.
The reason for maintaining the paddedBuffer all the time, regardless of whether it is necessary or not, is to avoid allocations on hot paths. However, I see at least two issues with padding in general. Firstly, it requires adding this extra branch. Secondly, it complicates the API: on one hand, the user doesn't need to be aware of it, but on the other hand, if they want to achieve the best performance, they should pad the input. Therefore, I've been considering removing the need for padding altogether. It should be possible, although I haven't thoroughly researched this topic.
To summarize: I'd start by verifying if removing the padding is possible. If so, I'd remove it and test the performance of the parser. If there is no regression compared to the current version with padding, we have a win-win situation.
Sorry, something went wrong.
There was a problem hiding this comment.
I think by design, the padding is 64 bytes. However, the 'paddedBuffer' is 34MB, that's such a waste. I'm just change the padding size to 64 bytes.
Sorry, something went wrong.
There was a problem hiding this comment.
I agree with you that this is a waste. However, in your approach, you are potentially allocating a new array on every call of the parse method, which can be costly.
I've been working on removing the padding entirely. It's a bit complicated, but we will see if it is feasible. I'll report back.
Sorry, something went wrong.
|
|
||
| int stringBufferLen = stringParser.getStringBufferIdx(); | ||
| byte[] newStringBuffer = new byte[stringBufferLen]; | ||
| System.arraycopy(stringBuffer, 0, newStringBuffer, 0, stringBufferLen); |
There was a problem hiding this comment.
Is this change related to #36? I'm asking because I'm a bit concerned that we need another allocation on the parsing path.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes. I think there must be an allocation somewhere, if we want to save the information of "old" data.
Sorry, something went wrong.
There was a problem hiding this comment.
And as I mentioned above, the default size of buffer and stringBuffer, as well as long[] tape in class Tape, is 34M. If we allocated 34M * 3 for each element, the cost is way too much.
Sorry, something went wrong.
In class JsonValue, the default size of buffer and stringBuffer, as well as long[] tape in class Tape, is 34M.
But in practice it's not necessary.
This patch reduce the size of them from 34M to its actual size.