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

Remove the superfluous Serializer interface and its implemetation #141

Merged
merged 1 commit into from
Oct 25, 2020

Conversation

ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Oct 24, 2020

No description provided.

@ppalaga
Copy link
Contributor Author

ppalaga commented Oct 24, 2020

The original MessageSerializer was using the same byte array for both read and write ops. Those reads and writes happen in different threads. To avoid potential conflicts between the read and write threads I have split the MessageSerializer into MessageReader and MessageWriter, each holding its own byte array buffer.

The issue is actually not critical at the moment, because the ops do not happen to interleave: client sends one message and then only receives without sending. This might perhaps change in the future, when we start supporting build cancellation. Anyway, I find the split conceptually cleaner.

That was about the first two commits

Now about the third commit, that I'd like to hear your opinion about:

Having done the split, I am struggling about the necessity of the byteBuf in the reader and writer

The byteBufs do not seem to make much sense performance-wise, because the underlying Data[Input|Output]Streams are are backed by custom Socket[Output|Input]Streams https://github.com/mvndaemon/mvnd/blob/ba56664e2cecafdb77981687b507bb82e57dd7ac/common/src/main/java/org/jboss/fuse/mvnd/common/DaemonConnection.java#L66-L67
that have a ByteBuffer https://github.com/mvndaemon/mvnd/blob/ba56664e2cecafdb77981687b507bb82e57dd7ac/common/src/main/java/org/jboss/fuse/mvnd/common/DaemonConnection.java#L159

So why could we not further simplify MessageReader and MessageWriter by removing the byteBuf and the [read|write]UTF methods and use java.io.Data[Inpiut|Output]Stream.[read|write]UTF(String) instead?
We'd end up with less code and in theory also a bit better perf. WDYT?

@gnodet
Copy link
Contributor

gnodet commented Oct 25, 2020

It seems this PR goes back and forth (adding classes + removing those).
At the end, it seems to be about removing the Serializer instance and make the code into static methods and removing the read/writeUTF methods to delegate to the ones from DataInput/OutputStream.
That sounds good to me, but we may want to simplify the commits a bit.

@ppalaga
Copy link
Contributor Author

ppalaga commented Oct 25, 2020

Thanks, squashed.

@ppalaga ppalaga changed the title Split MessageSerializer into reader and writer so that reading and writing threads do not share the same buffer Remove the superfluous Serializer interface and its implemetation Oct 25, 2020
@ppalaga ppalaga merged commit a71033c into apache:master Oct 25, 2020
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.

None yet

2 participants