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

Parametrize Connections to avoid type instabilities #983

Merged
merged 6 commits into from
Jan 6, 2023

Conversation

Drvi
Copy link
Collaborator

@Drvi Drvi commented Jan 5, 2023

I noticed that due to instabilities, all calls to bytesavailable were allocating for Connections due to the io field being abstractly typed (IO). Now these allocations are gone; a little parsing benchmark of a 500MB gzipped file:

# master
julia> @time streamedng(bucket, credentials, 4)
 18.736654 seconds (910.61 k allocations: 1.695 GiB)

# PR
julia> @time streamedng(bucket, credentials, 4)
 18.612146 seconds (552.58 k allocations: 1.695 GiB)

src/connectionpools.jl Outdated Show resolved Hide resolved
src/connectionpools.jl Outdated Show resolved Hide resolved
src/ConnectionPool.jl Outdated Show resolved Hide resolved
src/ConnectionPool.jl Show resolved Hide resolved
@Drvi Drvi changed the title Parametrize Connections to avoid type instabilities [WIP] Parametrize Connections to avoid type instabilities Jan 5, 2023
@Drvi
Copy link
Collaborator Author

Drvi commented Jan 6, 2023

@quinnj re the CI test failure on MacOS... locally, when I try running the Autobahn WebSocket Tests Servers I run into the following issue

...
Servers = [u'ws://127.0.0.1:9002']
Connection to ws://127.0.0.1:9002 failed (Connection was refused by other side: 111: Connection refused.)
...

and no report gets generated.
However, when I change the URL in test/websockets/config/fuzzingclient.json from "ws://127.0.0.1:9002" to "ws://host.docker.internal:9002", the tests are passing.

I don't understand why would this start failing as a result of the changes introduced in this PR, though... any ideas?

EDIT: Seems that the problem resolved itself

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2023

Codecov Report

Merging #983 (7808d88) into master (5057ad1) will increase coverage by 0.01%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
+ Coverage   82.56%   82.58%   +0.01%     
==========================================
  Files          36       36              
  Lines        3023     3026       +3     
==========================================
+ Hits         2496     2499       +3     
  Misses        527      527              
Impacted Files Coverage Δ
src/ConnectionPool.jl 85.65% <80.00%> (+0.18%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Drvi Drvi changed the title [WIP] Parametrize Connections to avoid type instabilities Parametrize Connections to avoid type instabilities Jan 6, 2023
Co-authored-by: Nick Robinson <[email protected]>
src/ConnectionPool.jl Outdated Show resolved Hide resolved
Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

Thanks @Drvi; this looks great. I'm going to commit the one suggestion I made so we get one more CI run, but then I'll merge.

@nickrobinson251
Copy link
Collaborator

Is there a simple allocations test we can add for this PR, so we don't lose these gains over time? something along the lines of @test @allocations(bytesavailable(conn)) == 0 ?

Copy link
Collaborator

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

an allocations-related test would be nice to have

otherwise looks great to me

@quinnj quinnj merged commit 1d843b9 into JuliaWeb:master Jan 6, 2023
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.

4 participants