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

Determine CHANNEL_SIZE from environment #171

Merged
merged 2 commits into from
Nov 23, 2017

Conversation

mschauer
Copy link
Contributor

Fixes #169 in the way proposed by @JobJob .

@JobJob
Copy link
Member

JobJob commented Nov 23, 2017

This is really good, but you're not based off master! Can you fix it up? prob easiest just to do it manually, or you can do this:

git format-patch -1 4eda8253161600e053fc4dbe3970980bf503b9b0 #sha of your commit
git checkout master
git pull
git branch channelsize2
git checkout channelsize2
patch -p1 < 0001-Determine-CHANNEL_SIZE-from-environment.patch

@mschauer
Copy link
Contributor Author

Ah, I started off with git checkout master without a fetch. Fixed here.

Copy link
Member

@JobJob JobJob left a comment

Choose a reason for hiding this comment

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

Ace!

src/core.jl Outdated
@@ -256,7 +256,8 @@ end
function async_push!(n, x, onerror=print_error)
taken = Base.n_avail(_messages)
if taken >= CHANNEL_SIZE
warn("Message queue is full. Ordering may be incorrect.")
warn("Message queue is full. Ordering may be incorrect. "*
"Channel size can be increased by setting `ENV[\"REACTIVE_CHANNEL_SIZE\"] = ...`")
Copy link
Member

@JobJob JobJob Nov 23, 2017

Choose a reason for hiding this comment

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

Nice touch 😄
maybe we should also add "by setting ENV[\"REACTIVE_CHANNEL_SIZE\"] = ... before using Reactive" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm..., that won't be enough because of pre-compilation freezing the value into the constant.

Copy link
Member

Choose a reason for hiding this comment

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

oh shit, you're right... maybe we need to change CHANNEL_SIZE to be a Ref ?

Copy link
Member

Choose a reason for hiding this comment

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

As in

const CHANNEL_SIZE = Ref(1024)
CHANNEL_SIZE[] = parse(Int, get(ENV, "REACTIVE_CHANNEL_SIZE", "1024"))

or something

Copy link
Member

@JobJob JobJob Nov 23, 2017

Choose a reason for hiding this comment

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

maybe haskey(ENV, "REACTIVE_CHANNEL_SIZE") && (CHANNEL_SIZE[] = parse(Int, ENV["REACTIVE_CHANNEL_SIZE"])) is better (less repetition of 1024)

edit: missed a )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the advantage of

const CHANNEL_SIZE = Ref(parse(Int, get(ENV, "REACTIVE_CHANNEL_SIZE", "1024")))

over

CHANNEL_SIZE = parse(Int, get(ENV, "REACTIVE_CHANNEL_SIZE", "1024"))

?

Copy link
Member

Choose a reason for hiding this comment

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

Because then we can set the Ref in the __init__() function. And it will be updated based on the current value of ENV["REACTIVE_CHANNEL_SIZE"] on using Reactive

Copy link
Member

Choose a reason for hiding this comment

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

I just tried out const CHANNEL_SIZE = Ref(1024)
and in __init__()

    haskey(ENV, "REACTIVE_CHANNEL_SIZE") && 
        (CHANNEL_SIZE[] = parse(Int, ENV["REACTIVE_CHANNEL_SIZE"]))
    runner_task[] = @async run()

plus changing lines ~226, ~257 from CHANNEL_SIZE to CHANNEL_SIZE[] and it seems to respect my ENV["REACTIVE_CHANNEL_SIZE"]

Copy link
Member

@JobJob JobJob Nov 23, 2017

Choose a reason for hiding this comment

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

Oh, I missed that your CHANNEL_SIZE above is not const. The const is an advantage for static compilation, and because non-const globals can be slow.

Copy link
Contributor Author

@mschauer mschauer Nov 23, 2017

Choose a reason for hiding this comment

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

Ah, I get it, const x = Ref makes sure that Julia can infer Int because only the value but not the type can change. JuliaLang/julia#13817 (comment)

src/core.jl Outdated
@@ -14,7 +14,7 @@ const runner_task = Ref{Task}()

const run_remove_dead_nodes = Ref(false)

const CHANNEL_SIZE = 1024
const CHANNEL_SIZE = parse(Int, get(ENV, "REACTIVE_CHANNEL_SIZE", "1024"))
Copy link
Member

Choose a reason for hiding this comment

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

Nice, didn't realise ENV converts to strings auto-magically.

@JobJob
Copy link
Member

JobJob commented Nov 23, 2017

Awesome. Thanks very much!

@JobJob JobJob merged commit 6001433 into JuliaGizmos:master Nov 23, 2017
@mschauer mschauer deleted the channelsize branch November 23, 2017 15: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