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

avoid kind type dispatch bugs in tuple_type_head and tuple_type_tail #27776

Merged
merged 2 commits into from
Jun 27, 2018

Conversation

jrevels
Copy link
Member

@jrevels jrevels commented Jun 25, 2018

This is a much more narrowly-scoped alternative to some of the tuple code changes that were originally in #27736 (i.e. c16a109).

It would still be nice eventually to figure out alternative implementations for c16a109, since switching to fieldtype is more sound in general (see discussion from #27736). However, for now, this much more palatable change works around the problems that are blocking Cassette.

Unfortunately, I don't have a non-Cassette test case for these bugs yet; they're pretty hard to work out MWEs for.

@jrevels
Copy link
Member Author

jrevels commented Jun 25, 2018

will merge tomorrow if there are no objections

@jrevels
Copy link
Member Author

jrevels commented Jun 25, 2018

(let's make sure nanosoldier doesn't object)

@nanosoldier runbenchmarks(ALL, vs = ":master")

@martinholters
Copy link
Member

I didn't follow along what issues this is solving, but ... can't they be tested for?

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@jrevels
Copy link
Member Author

jrevels commented Jun 26, 2018

I didn't follow along what issues this is solving, but ... can't they be tested for?

Definitely, it'd be great to have a non-Cassette MWE here, I just haven't figured one out yet. From the OP:

Unfortunately, I don't have a non-Cassette test case for these bugs yet; they're pretty hard to work out MWEs for.

If you're curious, here's a Cassette-based one I posted: #27736 (comment)

Looks like Nanosoldier doesn't like the new implementations...maybe adding a @_pure_meta to tuple_type_head will fix it? Let's see...

@jrevels
Copy link
Member Author

jrevels commented Jun 26, 2018

@nanosoldier runbenchmarks(ALL, vs = ":master")

@ararslan
Copy link
Member

Had to restart the server. @nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@jrevels
Copy link
Member Author

jrevels commented Jun 27, 2018

Cool, the @_pure_meta seemed to do the trick.

@jrevels jrevels merged commit 8a0a967 into master Jun 27, 2018
@jrevels jrevels deleted the jr/tuplekindtypefix2 branch June 27, 2018 01: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.

None yet

4 participants