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

change API to specify field assumptions at unpack callsite instead of globally #39

Merged
merged 8 commits into from
Oct 7, 2019

Conversation

jrevels
Copy link
Collaborator

@jrevels jrevels commented Sep 29, 2019

initial attempt at addressing #35. still needs:

  • performance checks
  • deprecation warnings for the old ImmutableStructType/MutableStructType
  • documentation updates

I realized that there are more assumptions at play here than just ordering; I couldn't think of a succinct term that precisely captures them all, though, so I just use the vague Exact{T} - open to better names if folks have ideas! EDIT: @ararslan used the word "strict" here which I kind of like

@jrevels
Copy link
Collaborator Author

jrevels commented Sep 30, 2019

sooooooo this definitely results in a performance hit; I'm messing around with a type that looks like:

on master:

  julia> @time unpack(bytes, Vector{MyType});
    0.025953 seconds (32.24 k allocations: 2.767 MiB)

  julia> @time unpack(bytes, Vector{MutableMyType});
    0.034484 seconds (24.18 k allocations: 2.644 MiB)

this PR:

julia> @time unpack(bytes, Vector{MyType}; strict=(MyType,));
  0.032173 seconds (32.25 k allocations: 2.767 MiB)

julia> @time unpack(bytes, Vector{MyType});
  0.049074 seconds (48.35 k allocations: 3.996 MiB)

where bytes was generated via pack(x::Vector{MyType}) where length(x) == 8058 and MyType is defined like:

struct MyType
    a::Vector{Symbol}
    b::Symbol
    c::Float64
    d::String
    e::UInt64
    f::Symbol
    g::Union{Nothing,Dict{Symbol,Any}}
end

(and MutableMyType has the same fields, but is mutable).

The API here is so much nicer (IMO) that I'm tempted to accept the performance hit...I'd bet that some additional optimization can be done to further recover performance for at least the strict path, but for the non-strict path I think this PR's approach might just be fundamentally slower than the old approach (assuming both approaches were optimized as much as possible, which still probably isn't the case).

@jrevels
Copy link
Collaborator Author

jrevels commented Oct 2, 2019

Okay, did another round of benchmarking; link to gist here.

It turns out that the fast path overhead for this PR compared to master is actually negligible as long as the types being passed in are inferred as constants! This is great: essentially, there's no performance loss compared to master for the situation that master currently supports (globally assuming a given type should be parsed strictly).

The mutable path is still a bit slower, but I feel more comfortable with being able to recover that performance back later without too much API hassle (even if it just means reintroducing master's current implementation, for those folks that absolutely need that level of performance).

Now that I'm happy enough with the performance situation, I'll crank away on the docs/depwarns to get this merged.

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