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

Remove some automatic conversion and comparison #338

Merged
merged 1 commit into from
Jan 1, 2023

Conversation

fingolfin
Copy link
Contributor

@fingolfin fingolfin commented Dec 28, 2022

  • Remove methods that allow comparing an object of type CxxRef{T} with one of type T. Note that other Ref types in Julia don't provide such methods either.
  • Remove convert methods from SmartPointer instances to to other types.

In both cases, the goal is to avoid invalidations. To give an idea of how bad the removed methods were, consider the number of invalidations as computed with this snippet in a fresh Julia session:

using SnoopCompileCore
invalidations = @snoopr using CxxWrap;
length(invalidations)

Before this patch, we get 20819 invalidations with Julia 1.8.4, and 13863 with Julia master.

After this patch, this is down to just 2695 in 1.8.4, respectively 1480 in Julia master.

The main issue with this patch is that it may break code relying on these convenience methods. However, the resulting failures should be easy to fix, and the fixed code should work fine with older CxxWrap versions. I would nevertheless suggest to mark this as a breaking change by following the appropriate semver rules.

This makes further progress towards resolving issue #278.

Resolves #313
Closes #334

@fingolfin
Copy link
Contributor Author

@barche I had to make minor modifications to the test suite, but now it passes. I also verified that the following packages using CxxWrap directly or indirectly still pass their test suites:

  • Casacore.jl
  • Oscar.jl
  • Polymake.jl
  • Singular.jl

I had trouble running the test suites of openPMD.jl and OpenSpiel.jl, but already without this PR, i.e. with the CxxWrap main branch (and also the latest release). This may be due to me running the tests on a mac, though... Weird.

In any case, based on my findings so far, I think it should be fine to merge this as long as the semver of the package is increased afterwards to indicate to a breaking change.

- Remove methods that allow comparing an object of type `CxxRef{T}` with
  one of type `T`. Note that other `Ref` types in Julia don't provide such
  methods either.
- Remove `convert` methods from `SmartPointer` instances to to other types.

In both cases, the goal is to avoid invalidations. To give an idea of how
bad the removed methods were, consider the number of invalidations as
computed with this snippet in a fresh Julia session:

    using SnoopCompileCore
    invalidations = @Snoopr using CxxWrap;
    length(invalidations)

Before this patch, we get 20819 invalidations with Julia 1.8.4, and
13863 with Julia master.

After this patch, this is down to just 2695 in 1.8.4, respectively
1480 in Julia master.

The main issue with this patch is that it may break code relying on
these convenience methods. However, the resulting failures should be
easy to fix, and the fixed code should work fine with older CxxWrap
versions. I would nevertheless suggest to mark this as a breaking change
by following the appropriate semver rules.
@fingolfin fingolfin force-pushed the mh/radical-invalidations branch from 69417e4 to 19c9051 Compare December 29, 2022 13:20
@fingolfin fingolfin closed this Dec 29, 2022
@fingolfin fingolfin reopened this Dec 29, 2022
@fingolfin fingolfin mentioned this pull request Dec 31, 2022
@barche barche merged commit 4bb56bb into JuliaInterop:main Jan 1, 2023
@barche
Copy link
Collaborator

barche commented Jan 1, 2023

Thanks for this, causes no issues at all for QML.jl either.

@fingolfin fingolfin deleted the mh/radical-invalidations branch January 1, 2023 16:11
@fingolfin
Copy link
Contributor Author

Great! Now just need CxxWrap.jl 0.13 to be released... well, and I am off to tracking down the next source of invalidations ;-)

@dannys4 dannys4 mentioned this pull request Jan 12, 2024
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.

Strange convert method
2 participants