Change isnull field of Nullable into hasvalue by nalimilan · Pull Request #18510 · JuliaLang/julia

nalimilan · 2016-09-14T20:04:44Z

This is consistent with most other languages and formats.

tkelman · 2016-09-14T21:42:42Z

base/nullable.jl

 end

-Nullable{T}(value::T, isnull::Bool=false) = Nullable{T}(value, isnull)
+Nullable{T}(value::T, hasvalue::Bool=true) = Nullable{T}(value, hasvalue)


xref #18511 (comment) cc @kshyatt

is the second argument meant to be used?

@tkelman Indeed the two-argument form is mostly useful for tests, where using === requires controlling the contents of value (cf. #16923). Maybe it can also help for performance, instead of writing ifelse(hasvalue, Nullable(value), Nullable()), I would have to check. I guess @johnmyleswhite can tell.

We make use of this for null-safe operations where we can generate the value and hasvalue fields without branching.

okay, then it should be documented?

Yes. We should document it in such a way that it's clear that you have more freedom than you usually need and that its use is a performance optimization.

TotalVerb · 2016-09-15T01:14:13Z

base/nullable.jl

 @inline function get{S,T}(x::Nullable{S}, y::T)
    if isbits(S)
-        ifelse(x.isnull, y, x.value)
+        ifelse(isnull(x), y, x.value)


Should we maybe run benchmarks to make sure any ! operations are successfully eliminated? Or perhaps this is nothing compared to the cost of the branch, so even if they aren't removed, it doesn't matter.

Not a bad idea in general, but I don't think we have any benchmarks for Nullable yet. We really need to add some. We could also check the generated code manually in some cases.

I've opened a PR to add benchmarks: JuliaCI/BaseBenchmarks.jl#24. Please suggest cases to add measurements for.

I see no cause for concern in the generated code. It has not changed at all with this patch.

julia> @code_llvm get(Nullable{Int}(10), 100) define i64 @julia_get_64687(%Nullable.7*, i64) #0 { top: %2 = getelementptr inbounds %Nullable.7, %Nullable.7* %0, i64 0, i32 0 %3 = load i8, i8* %2, align 1 %4 = and i8 %3, 1 %5 = icmp eq i8 %4, 0 %6 = getelementptr inbounds %Nullable.7, %Nullable.7* %0, i64 0, i32 1 %7 = load i64, i64* %6, align 8 %8 = select i1 %5, i64 %7, i64 %1 ret i64 %8 }

nalimilan · 2016-09-15T11:30:26Z

The first commit only changes the field but keeps the constructor as it was, while the second also change the constructor. I think we want both for consistency, but separating them helps measuring the breakage it creates. Overall, very little changes are needed, which is promising as regards the broader ecosystem.

The two most disruptive cases are:

the two-argument constructor (if we change it); see thread above
the C API, where the isnull field is always passed explicitly (no constructor default value)

Luckily, these two cases appear to be relatively rare.

TotalVerb

Looks great. Since generated code for get has not changed, I suspect there will be no performance regressions (except possibly isnull, but that's unavoidable).

This is consistent with most other languages and formats.

This method is not currently documented, and the new form is more consistent with the new structure of the type.

nalimilan · 2016-09-25T13:18:17Z

I've run the benchmarks locally. As expected, they are slower with this PR if we keep the isnull field of NullableArray, but when reversing it to match hasvalue most regressions go away. What remains are regression in perf_countnulls, which is kind of logical since all it does is call isnull. I don't see this as a problem since it real cases, we generally have a branch/ifelse which the compiler automatically reverses for avoid the negation. We could change that benchmark into count_nonnulls, or add it in parallel; doesn't matter much IMHO.

So now it remains to find out whether many packages would be broken by the second commit. I've made a PR against NullableArrays, and it wasn't too painful. I think most packages should work fine.

@tkelman Could you please run PkgEval on this PR? Thanks!

tkelman · 2016-09-27T10:31:05Z

Sure. I messed something up in a rebase of my first attempt, running again now.

tkelman · 2016-09-28T06:06:40Z

Shouldn't be too surprised by this: CategoricalArrays, CSV, and SQLite are new failures. Git was a timeout, DynamicDiscreteModels is flaky. https://gist.github.com/7d1882b630429019667fece0cc82172f

nalimilan · 2016-09-28T07:55:05Z

Great! These are all packages which were supposed to fail, and whose maintainers are in the loop. Will have a look. Cc: @quinnj

quinnj · 2016-09-28T09:57:31Z

Noted. Will this be backported? Or just on 0.6?

nalimilan · 2016-09-28T09:57:52Z

No, just on 0.6.

quinnj · 2016-09-28T10:22:14Z

Great. Merge away. Should be a pretty easy compat.

nalimilan · 2016-09-29T07:35:33Z

Should we merge this then? @johnmyleswhite? @davidagold?

johnmyleswhite

Good by me.

johnmyleswhite · 2016-09-29T15:09:13Z

base/nullable.jl

 end

-Nullable{T}(value::T, isnull::Bool=false) = Nullable{T}(value, isnull)
+Nullable{T}(value::T, hasvalue::Bool=true) = Nullable{T}(value, hasvalue)


We make use of this for null-safe operations where we can generate the value and hasvalue fields without branching.

Compat for Julia #18510, #18484

nalimilan added the missing data Base.missing and related functionality label Sep 14, 2016

nalimilan force-pushed the nl/hasvalue branch 3 times, most recently from 2618e99 to 8da3c10 Compare September 14, 2016 21:09

tkelman reviewed Sep 14, 2016

View reviewed changes

TotalVerb reviewed Sep 15, 2016

View reviewed changes

nalimilan force-pushed the nl/hasvalue branch from 2ba180a to fcd065d Compare September 15, 2016 06:21

nalimilan mentioned this pull request Sep 15, 2016

[WIP]/[RFC] Move EVEN MORE things out of helpDB #18511

Closed

nalimilan force-pushed the nl/hasvalue branch from fcd065d to 672c01c Compare September 15, 2016 07:22

TotalVerb approved these changes Sep 16, 2016

View reviewed changes

This was referenced Sep 18, 2016

Add generic isnull and unsafe_get methods #18484

Merged

Add benchmarks for Nullable JuliaCI/BaseBenchmarks.jl#24

Merged

nalimilan added 2 commits September 23, 2016 17:14

Change isnull field of Nullable into hasvalue

f9e34c3

This is consistent with most other languages and formats.

Change isnull argument to Nullable() constructor to hasvalue

32a08c7

This method is not currently documented, and the new form is more consistent with the new structure of the type.

nalimilan force-pushed the nl/hasvalue branch from 672c01c to 32a08c7 Compare September 23, 2016 16:15

nalimilan mentioned this pull request Sep 24, 2016

Prepare for change from isnull to hasvalue field in Base JuliaStats/NullableArrays.jl#150

Merged

nalimilan mentioned this pull request Sep 27, 2016

Update to use NullableArrays and CategoricalArrays JuliaInterop/RCall.jl#145

Merged

2 tasks

johnmyleswhite approved these changes Sep 29, 2016

View reviewed changes

nalimilan merged commit b36141f into master Sep 29, 2016

nalimilan deleted the nl/hasvalue branch September 29, 2016 16:09

davidagold mentioned this pull request Oct 17, 2016

Compat for Julia #18510, #18484 JuliaLang/Compat.jl#287

Merged

StefanKarpinski pushed a commit that referenced this pull request Feb 8, 2018

Compat for Julia #18510, #18484

11b9210

StefanKarpinski pushed a commit that referenced this pull request Feb 8, 2018

Merge pull request #287 from davidagold/dag/nullable

2534435

Compat for Julia #18510, #18484

Uh oh!

Conversation

nalimilan commented Sep 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TotalVerb Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalimilan commented Sep 15, 2016

Uh oh!

TotalVerb left a comment

Choose a reason for hiding this comment

Uh oh!

nalimilan commented Sep 25, 2016

Uh oh!

tkelman commented Sep 27, 2016

Uh oh!

tkelman commented Sep 28, 2016

Uh oh!

nalimilan commented Sep 28, 2016

Uh oh!

quinnj commented Sep 28, 2016

Uh oh!

nalimilan commented Sep 28, 2016

Uh oh!

quinnj commented Sep 28, 2016

Uh oh!

nalimilan commented Sep 29, 2016

Uh oh!

johnmyleswhite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TotalVerb Sep 15, 2016 •

edited

Loading