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

Optimize socket read/write with array ptrs #85

Merged
merged 4 commits into from
Nov 19, 2022

Conversation

armanbilge
Copy link
Owner

This is a nice optimization :)

@armanbilge
Copy link
Owner Author

Um ... @LeeTibbert do you have any idea what might be going on? 😅 I don't see how this can be related to these changes.

[info] ==> X epollcat.TcpSuite.local and remote addresses 0.00s munit.ComparisonFailException: /Users/runner/work/epollcat/epollcat/tests/shared/src/test/scala/epollcat/TcpSuite.scala:133
[info] 132:                assertEquals(clientRemote, serverLocal)
[info] 133:                assertEquals(serverRemote, clientLocal)
[info] 134:              }
[info] values are not the same
[info] => Obtained
[info] /127.0.0.1:49866
[info] => Diff (- obtained, + expected)
[info] -/127.0.0.1:49866
[info] +/0:0:0:0:0:0:7F00:1:49866

https://github.com/armanbilge/epollcat/actions/runs/3432632786/jobs/5722112147

I am going to try re-running CI.

@armanbilge
Copy link
Owner Author

Lovely. Seems to be sporadic 🙃

@LeeTibbert
Copy link
Collaborator

Sorry for the belated reply, the at_mention must have slipped past me.

I suspect this is using SN 0.4.8 and the InetAddress changes in it. True?

I will have to trace. First thing to check is why an IPv4 address is being represented
in the number of bytes for and IPv6 address. Should be either 4 bytes (IPv4) or converted
to "::FFFF:" form.

@armanbilge armanbilge closed this Nov 18, 2022
@armanbilge armanbilge reopened this Nov 18, 2022
@LeeTibbert
Copy link
Collaborator

Re-opened?? I thought Issue #88 had been solved.

@armanbilge
Copy link
Owner Author

@LeeTibbert the purpose of this PR was completely unrelated, it is just where I first stumbled on the "quirk".

If you have a chance, I would appreciate a review 😄

@LeeTibbert
Copy link
Collaborator

OK back to the program (PR) in progress. I will take a look at the changed file.

@LeeTibbert
Copy link
Collaborator

This LGTM

I did a minimal trace on the two "go" tailrec methods because it after
the first two or three readings, it looked like they had moved on the page
and not had any substantive changes.

Using the bulk ByteBuffer methods looks like it should speed things up.
It nothing else, it simplifies the code and makes it easier to follow.

Copy link
Collaborator

@LeeTibbert LeeTibbert left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you for the opportunity to review

@armanbilge
Copy link
Owner Author

it looked like they had moved on the page
and not had any substantive changes.

For future reference: I often find it helpful to review with whitespace-only changes "hidden". Helps me identify the real changes and not get lost in indentation :)

Screen Shot 2022-11-18 at 4 53 18 PM

@LeeTibbert
Copy link
Collaborator

Good info. I had thought they had moved up & down the page, but I may have been looking at
indentation changes.

@armanbilge
Copy link
Owner Author

Thanks for the review! Much appreciated, and for all your work on the quirk :)

@armanbilge armanbilge merged commit 9c00d8c into main Nov 19, 2022
@armanbilge armanbilge deleted the pr/array-ptr-optimizations branch November 19, 2022 00:58
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.

2 participants