Migrate off Unsafe (guava-android) #7742
Assignees
Labels
Comments
|
I posted some thoughts in https://issuetracker.google.com/issues/134377851#comment3. |
copybara-service bot
pushed a commit
that referenced
this issue
Mar 28, 2025
Nearly all access should be performed through the various methods (e.g., `casListeners(...)`, `listeners()`). In fact, we could be principled and perform _all_ access through those methods (except of course for within the implementations of those methods themselves). I haven't done that here just out of fear that it will somehow affect performance or cause stack overflows. Accidental usage has been something that I've historically worried about in, e.g., `AbstractFuture.set(V value)`, whose parameter has had the same name as a field. That _particular_ example matters less at the moment because the field recently became `private` to a new superclass, `AbstractFutureState`, and so it's not accessible in the subclass `AbstractFuture`. But it's going to matter again: I'm likely to make the fields package-private as part of [work to migrate `guava-android` off `Unsafe`](#7742). Currently, the fields can be `private` because we call `MethodHandles.lookup()` from within `AbsractFutureState`. (Yes, it would initially seem that [we shouldn't have to](https://github.com/google/guava/blob/c7363f7fb40698bb5f99d198cc45884f38642f86/guava/src/com/google/common/util/concurrent/AbstractFutureState.java#L612-L632), but we do.) But that requires `AbstractFutureState` to refer to `MethodHandles.Lookup` in one of its method signatures\[*\], and that makes old versions of Android upset. To avoid that, I will make `value` package-private, at which point I won't need the `MethodHandles.Lookup` reference in `AbstractFutureState` itself. And when I tried making `value` package-private, _one test_ started to fail. It should have taken me less time to figure out, but I eventually discovered that the problem was that [the test refers to "`value`" inside an `AbstractFuture` subclass](https://github.com/google/guava/blob/c7363f7fb40698bb5f99d198cc45884f38642f86/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java#L78). Previously, this referred to the local variable `value` from the test method; with my change, it was instead referring to the `value` field. (I wouldn't have to have gone down the road of making the field non-`private` in the first place [if not for Java 8 compatibility](#6614 (comment)).... Still, as discussed above, this rename could protect against problems _within_ the file, too, and such problems could arise even if the field were to remain `private`.) \[*\] Or maybe I could declare the method as returning `Object` instead of `MethodHandles.Lookup`, and the caller could cast it back? But we found during [our `SequencedCollection` work](#6903) that Android (and I think maybe the JVM, but I can't find my record of this) can produce a `VerifyError` in some cases in which _implementation_ code refers to an unknown type, I think specifically when it needs to check whether a `return someThingOfTypeFoo` from that method is compatible with the return type `Bar` of the method. We _might_ be able to work around that by performing an explicit, "redundant" cast to `Object`, but I'm not even sure how to get javac to do that, and it feels very fragile, especially in the presence of optimization/minification tools. RELNOTES=n/a PiperOrigin-RevId: 741511657
copybara-service bot
pushed a commit
that referenced
this issue
Mar 28, 2025
Nearly all access should be performed through the various methods (e.g., `casListeners(...)`, `listeners()`). In fact, we could be principled and perform _all_ access through those methods (except of course for within the implementations of those methods themselves). I haven't done that here just out of fear that it will somehow affect performance or cause stack overflows. Accidental usage has been something that I've historically worried about in, e.g., `AbstractFuture.set(V value)`, whose parameter has had the same name as a field. That _particular_ example matters less at the moment because the field recently became `private` to a new superclass, `AbstractFutureState`, and so it's not accessible in the subclass `AbstractFuture`. But it's going to matter again: I'm likely to make the fields package-private as part of [work to migrate `guava-android` off `Unsafe`](#7742). Currently, the fields can be `private` because we call `MethodHandles.lookup()` from within `AbsractFutureState`. (Yes, it would initially seem that [we shouldn't have to](https://github.com/google/guava/blob/c7363f7fb40698bb5f99d198cc45884f38642f86/guava/src/com/google/common/util/concurrent/AbstractFutureState.java#L612-L632), but we do.) But that requires `AbstractFutureState` to refer to `MethodHandles.Lookup` in one of its method signatures\[*\], and that makes old versions of Android upset. To avoid that, I will make `value` package-private, at which point I won't need the `MethodHandles.Lookup` reference in `AbstractFutureState` itself. And when I tried making `value` package-private, _one test_ started to fail. It should have taken me less time to figure out, but I eventually discovered that the problem was that [the test refers to "`value`" inside an `AbstractFuture` subclass](https://github.com/google/guava/blob/c7363f7fb40698bb5f99d198cc45884f38642f86/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java#L78). Previously, this referred to the local variable `value` from the test method; with my change, it was instead referring to the `value` field. (I wouldn't have to have gone down the road of making the field non-`private` in the first place [if not for Java 8 compatibility](#6614 (comment)).... Still, as discussed above, this rename could protect against problems _within_ the file, too, and such problems could arise even if the field were to remain `private`.) \[*\] Or maybe I could declare the method as returning `Object` instead of `MethodHandles.Lookup`, and the caller could cast it back? But we found during [our `SequencedCollection` work](#6903) that Android (and I think maybe the JVM, but I can't find my record of this) can produce a `VerifyError` in some cases in which _implementation_ code refers to an unknown type, I think specifically when it needs to check whether a `return someThingOfTypeFoo` from that method is compatible with the return type `Bar` of the method. We _might_ be able to work around that by performing an explicit, "redundant" cast to `Object`, but I'm not even sure how to get javac to do that, and it feels very fragile, especially in the presence of optimization/minification tools. RELNOTES=n/a PiperOrigin-RevId: 741511657
copybara-service bot
pushed a commit
that referenced
this issue
Mar 28, 2025
Nearly all access should be performed through the various methods (e.g., `casListeners(...)`, `listeners()`). In fact, we could be principled and perform _all_ access through those methods (except of course for within the implementations of those methods themselves). I haven't done that here just out of fear that it will somehow affect performance or cause stack overflows. Accidental usage has been something that I've historically worried about in, e.g., `AbstractFuture.set(V value)`, whose parameter has had the same name as a field. That _particular_ example matters less at the moment because the field recently became `private` to a new superclass, `AbstractFutureState`, and so it's not accessible in the subclass `AbstractFuture`. But it's going to matter again: I'm likely to make the fields package-private as part of [work to migrate `guava-android` off `Unsafe`](#7742). Currently, the fields can be `private` because we call `MethodHandles.lookup()` from within `AbsractFutureState`. (Yes, it would initially seem that [we shouldn't have to](https://github.com/google/guava/blob/c7363f7fb40698bb5f99d198cc45884f38642f86/guava/src/com/google/common/util/concurrent/AbstractFutureState.java#L612-L632), but we do.) But that requires `AbstractFutureState` to refer to `MethodHandles.Lookup` in one of its method signatures\[*\], and that makes old versions of Android upset. To avoid that, I will make `value` package-private, at which point I won't need the `MethodHandles.Lookup` reference in `AbstractFutureState` itself. And when I tried making `value` package-private, _one test_ started to fail. It should have taken me less time to figure out, but I eventually discovered that the problem was that [the test refers to "`value`" inside an `AbstractFuture` subclass](https://github.com/google/guava/blob/c7363f7fb40698bb5f99d198cc45884f38642f86/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java#L78). Previously, this referred to the local variable `value` from the test method; with my change, it was instead referring to the `value` field. (I wouldn't have to have gone down the road of making the field non-`private` in the first place [if not for Java 8 compatibility](#6614 (comment)).... Still, as discussed above, this rename could protect against problems _within_ the file, too, and such problems could arise even if the field were to remain `private`.) \[*\] Or maybe I could declare the method as returning `Object` instead of `MethodHandles.Lookup`, and the caller could cast it back? But we found during [our `SequencedCollection` work](#6903) that Android (and I think maybe the JVM, but I can't find my record of this) can produce a `VerifyError` in some cases in which _implementation_ code refers to an unknown type, I think specifically when it needs to check whether a `return someThingOfTypeFoo` from that method is compatible with the return type `Bar` of the method. We _might_ be able to work around that by performing an explicit, "redundant" cast to `Object`, but I'm not even sure how to get javac to do that, and it feels very fragile, especially in the presence of optimization/minification tools. RELNOTES=n/a PiperOrigin-RevId: 741607075
copybara-service bot
pushed a commit
that referenced
this issue
Mar 31, 2025
By keeping them `private`, we've mostly been making our lives more complicated, as discussed in cl/741607075. Currently, that would complicate using `VarHandle` in `guava-android`, and that would in turn complicate [our migration off `Unsafe`](#7742). (But then making them _package-private_ complicated things too, as seen in cl/742311780....) This CL continues the trend of work that is necessary [only because of Java 8 compatibility](#6614 (comment)). RELNOTES=n/a PiperOrigin-RevId: 741607263
copybara-service bot
pushed a commit
that referenced
this issue
Mar 31, 2025
By keeping them `private`, we've mostly been making our lives more complicated, as discussed in cl/741607075. Currently, that would complicate using `VarHandle` in `guava-android`, and that would in turn complicate [our migration off `Unsafe`](#7742). (But then making them _package-private_ complicated things too, as seen in cl/742311780....) This CL continues the trend of work that is necessary [only because of Java 8 compatibility](#6614 (comment)). RELNOTES=n/a PiperOrigin-RevId: 741607263
copybara-service bot
pushed a commit
that referenced
this issue
Mar 31, 2025
By keeping them `private`, we've mostly been making our lives more complicated, as discussed in cl/741607075. Currently, that would complicate using `VarHandle` in `guava-android`, and that would in turn complicate [our migration off `Unsafe`](#7742). (But then making them _package-private_ complicated things too, as seen in cl/742311780....) This CL continues the trend of work that is necessary [only because of Java 8 compatibility](#6614 (comment)). RELNOTES=n/a PiperOrigin-RevId: 742334547
copybara-service bot
pushed a commit
that referenced
this issue
Apr 1, 2025
This test doesn't exercise any alternative `AtomicHelper` implementations; it merely checks that `AbstractFutureState` selects the implementation that we expect under each given environment. This CL is prompted by my discovery today that I'd gotten a Proguard config wrong. I corrected the config in cl/742724817, but this new test suggests that (at least in under our monorepo's build toolchain, which differs from typical external Android toolchains!) `AbstractFutureState` continued doing the right thing throughout, even under optimization/minification (which I tested locally), apparently because the optimizer autodetected that we were reflecting on the fields. Still, we shouldn't rely on that. I also cleaned up the existing `AtomicHelper` tests to use a package-private method instead of reading a `private` field. The motivation for that change is that my new test was failing under minification because the class name did not match the expected value. That forced me to expose something more stable about the choice of `AtomicHelper` to the tests. And once I had that, I figured that I might as well use it in the existing tests. Maybe someday I'll update `AggregateFutureStateFallbackAtomicHelperTest` similarly. This CL continues [work to migrate `guava-android` off `Unsafe`](#7742). RELNOTES=n/a PiperOrigin-RevId: 742695029
copybara-service bot
pushed a commit
that referenced
this issue
Apr 1, 2025
This test doesn't exercise any alternative `AtomicHelper` implementations; it merely checks that `AbstractFutureState` selects the implementation that we expect under each given environment. This CL is prompted by my discovery today that I'd gotten a Proguard config wrong. I corrected the config in cl/742724817, but this new test suggests that (at least in under our monorepo's build toolchain, which differs from typical external Android toolchains!) `AbstractFutureState` continued doing the right thing throughout, even under optimization/minification (which I tested locally), apparently because the optimizer autodetected that we were reflecting on the fields. Still, we shouldn't rely on that. I also cleaned up the existing `AtomicHelper` tests to use a package-private method instead of reading a `private` field. The motivation for that change is that my new test was failing under minification because the class name did not match the expected value. That forced me to expose something more stable about the choice of `AtomicHelper` to the tests. And once I had that, I figured that I might as well use it in the existing tests. Maybe someday I'll update `AggregateFutureStateFallbackAtomicHelperTest` similarly. This CL continues [work to migrate `guava-android` off `Unsafe`](#7742). RELNOTES=n/a PiperOrigin-RevId: 742859752
copybara-service bot
pushed a commit
that referenced
this issue
Apr 1, 2025
…n run under the JVM. Under Android, I still don't have us even _try_ `VarHandle`: - I'm not sure that using `VarHandle` is possible with our current internal Android build setup, as discussed in the internal commentary on cl/711733182. - Using `VarHandle` there may at least somewhat more complex: My testing suggests that some versions of Android expose `VarHandle` to our reflective check but don't actually expose `MethodHandles.Lookup.findVarHandle`. Accordingly, sgjesse@ [recommends using a check on `SDK_INT`](https://issuetracker.google.com/issues/134377851#comment4) ([33](https://developer.android.com/reference/java/lang/invoke/MethodHandles.Lookup#findVarHandle(java.lang.Class%3C?%3E,%20java.lang.String,%20java.lang.Class%3C?%3E)) or higher) instead of reflection under Android. Since `guava-android` can be used under the JVM, too, we'd need to use a combination of the SDK check _and_ reflection. And we'd need to perform the `SDK_INT` check reflectively, as [we do in `TempFileCreator`](https://github.com/google/guava/blob/266ce0022a1c54244df1af0bde81116ed7703577/guava/src/com/google/common/io/TempFileCreator.java#L75) (which, hmm, I should clean up as obsolete one of these days...). (We could at least _reduce_ the amount of reflection we need if we were to depend on the Android SDK at build time, as discussed in b/403282918.) - I have no idea whether `VarHandle` is faster or slower than `Unsafe` there (and I can't easily tell because of the build problems above). - I'm not aware of efforts to remove `Unsafe` under Android. This CL has two advantages for JVM users of `guava-android`: - It eliminates a warning about usage of `Unsafe` under newer JDKs. Note that `AbstractFuture` _already_ has run correctly if access to `Unsafe` is outright disallowed (as with `-sun-misc-unsafe-memory-access=deny`): It detects this and falls back to an alternative implementation. However, if `Unsafe` is available but produces warnings, `guava-android` would use it, triggering those warnings. This CL makes Guava not even try to use it under newer JVMs because it now tries `VarHandle` first. - `VarHandle` may be marginally faster than `Unsafe` under the JVM, as discussed in cl/711733182. (It also doesn't lead to VM crashes if you manage to pass `null` where you shouldn't, as I did back in b/397641020 :) But that's more something that's nice for us as Guava developers, not something that's nice for Guava _users_.) This CL is probably the most _prominent_ part of [our migration of `guava-android` off `Unsafe`](#7742). It is debatable whether it is the most _important_, given that [at least one class, `Striped64`, does not seem to have a fallback at all if `Unsafe` is unavailable](#6806 (comment)). Still, this CL addresses the warning that most users are seeing, and it gives us some precedent for how to deal with `Striped64`. Finally, it looks like our existing tests for `VarHandle` had [a mismatch between the context class loader and the class loader that we load `AbstractFutureTest` in](https://github.com/google/guava/blob/266ce0022a1c54244df1af0bde81116ed7703577/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java#L105-L107)? That seems fairly bad. This CL fixes it, extracting a method to guard against future such mismatches. Out of an abundance of caution, I made a similar change in `AggregateFutureStateFallbackAtomicHelperTest`, even though there's not really an opportunity for a mismatch there, given that there's only one alternative class loader. RELNOTES=`util.concurrent`: Changed the `guava-android` copy of `AbstractFuture` to try `VarHandle` before `Unsafe`, eliminating a warning under newer JDKs. PiperOrigin-RevId: 741611120
copybara-service bot
pushed a commit
that referenced
this issue
Apr 2, 2025
…n run under the JVM. Under Android, I still don't have us even _try_ `VarHandle`: - I'm not sure that using `VarHandle` is possible with our current internal Android build setup, as discussed in the internal commentary on cl/711733182. - Using `VarHandle` there may at least somewhat more complex: My testing suggests that some versions of Android expose `VarHandle` to our reflective check but don't actually expose `MethodHandles.Lookup.findVarHandle`. Accordingly, sgjesse@ [recommends using a check on `SDK_INT`](https://issuetracker.google.com/issues/134377851#comment4) ([33](https://developer.android.com/reference/java/lang/invoke/MethodHandles.Lookup#findVarHandle(java.lang.Class%3C?%3E,%20java.lang.String,%20java.lang.Class%3C?%3E)) or higher) instead of reflection under Android. Since `guava-android` can be used under the JVM, too, we'd need to use a combination of the SDK check _and_ reflection. And we'd need to perform the `SDK_INT` check reflectively, as [we do in `TempFileCreator`](https://github.com/google/guava/blob/266ce0022a1c54244df1af0bde81116ed7703577/guava/src/com/google/common/io/TempFileCreator.java#L75) (which, hmm, I should clean up as obsolete one of these days...). (We could at least _reduce_ the amount of reflection we need if we were to depend on the Android SDK at build time, as discussed in b/403282918.) - I have no idea whether `VarHandle` is faster or slower than `Unsafe` there (and I can't easily tell because of the build problems above). - I'm not aware of efforts to remove `Unsafe` under Android. This CL has two advantages for JVM users of `guava-android`: - It eliminates a warning about usage of `Unsafe` under newer JDKs. Note that `AbstractFuture` _already_ has run correctly if access to `Unsafe` is outright disallowed (as with `-sun-misc-unsafe-memory-access=deny`): It detects this and falls back to an alternative implementation. However, if `Unsafe` is available but produces warnings, `guava-android` would use it, triggering those warnings. This CL makes Guava not even try to use it under newer JVMs because it now tries `VarHandle` first. - `VarHandle` may be marginally faster than `Unsafe` under the JVM, as discussed in cl/711733182. (It also doesn't lead to VM crashes if you manage to pass `null` where you shouldn't, as I did back in b/397641020 :) But that's more something that's nice for us as Guava developers, not something that's nice for Guava _users_.) This CL is probably the most _prominent_ part of [our migration of `guava-android` off `Unsafe`](#7742). It is debatable whether it is the most _important_, given that [at least one class, `Striped64`, does not seem to have a fallback at all if `Unsafe` is unavailable](#6806 (comment)). Still, this CL addresses the warning that most users are seeing, and it gives us some precedent for how to deal with `Striped64`. Finally, it looks like our existing tests for `VarHandle` had [a mismatch between the context class loader and the class loader that we load `AbstractFutureTest` in](https://github.com/google/guava/blob/266ce0022a1c54244df1af0bde81116ed7703577/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java#L105-L107)? That seems fairly bad. This CL fixes it, extracting a method to guard against future such mismatches. Out of an abundance of caution, I made a similar change in `AggregateFutureStateFallbackAtomicHelperTest`, even though there's not really an opportunity for a mismatch there, given that there's only one alternative class loader. RELNOTES=`util.concurrent`: Changed the `guava-android` copy of `AbstractFuture` to try `VarHandle` before `Unsafe`, eliminating a warning under newer JDKs. PiperOrigin-RevId: 741611120
copybara-service bot
pushed a commit
that referenced
this issue
Apr 2, 2025
…n run under the JVM. Under Android, I still don't have us even _try_ `VarHandle`: - I'm not sure that using `VarHandle` is possible with our current internal Android build setup, as discussed in the internal commentary on cl/711733182. - Using `VarHandle` there may at least somewhat more complex: My testing suggests that some versions of Android expose `VarHandle` to our reflective check but don't actually expose `MethodHandles.Lookup.findVarHandle`. Accordingly, sgjesse@ [recommends using a check on `SDK_INT`](https://issuetracker.google.com/issues/134377851#comment4) ([33](https://developer.android.com/reference/java/lang/invoke/MethodHandles.Lookup#findVarHandle(java.lang.Class%3C?%3E,%20java.lang.String,%20java.lang.Class%3C?%3E)) or higher) instead of reflection under Android. Since `guava-android` can be used under the JVM, too, we'd need to use a combination of the SDK check _and_ reflection. And we'd need to perform the `SDK_INT` check reflectively, as [we do in `TempFileCreator`](https://github.com/google/guava/blob/266ce0022a1c54244df1af0bde81116ed7703577/guava/src/com/google/common/io/TempFileCreator.java#L75) (which, hmm, I should clean up as obsolete one of these days...). (We could at least _reduce_ the amount of reflection we need if we were to depend on the Android SDK at build time, as discussed in b/403282918.) - I have no idea whether `VarHandle` is faster or slower than `Unsafe` there (and I can't easily tell because of the build problems above). - I'm not aware of efforts to remove `Unsafe` under Android. This CL has two advantages for JVM users of `guava-android`: - It eliminates a warning about usage of `Unsafe` under newer JDKs. Note that `AbstractFuture` _already_ has run correctly if access to `Unsafe` is outright disallowed (as with `-sun-misc-unsafe-memory-access=deny`): It detects this and falls back to an alternative implementation. However, if `Unsafe` is available but produces warnings, `guava-android` would use it, triggering those warnings. This CL makes Guava not even try to use it under newer JVMs because it now tries `VarHandle` first. - `VarHandle` may be marginally faster than `Unsafe` under the JVM, as discussed in cl/711733182. (It also doesn't lead to VM crashes if you manage to pass `null` where you shouldn't, as I did back in b/397641020 :) But that's more something that's nice for us as Guava developers, not something that's nice for Guava _users_.) This CL is probably the most _prominent_ part of [our migration of `guava-android` off `Unsafe`](#7742). It is debatable whether it is the most _important_, given that [at least one class, `Striped64`, does not seem to have a fallback at all if `Unsafe` is unavailable](#6806 (comment)). Still, this CL addresses the warning that most users are seeing, and it gives us some precedent for how to deal with `Striped64`. Finally, it looks like our existing tests for `VarHandle` had [a mismatch between the context class loader and the class loader that we load `AbstractFutureTest` in](https://github.com/google/guava/blob/266ce0022a1c54244df1af0bde81116ed7703577/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java#L105-L107)? That seems fairly bad. This CL fixes it, extracting a method to guard against future such mismatches. Out of an abundance of caution, I made a similar change in `AggregateFutureStateFallbackAtomicHelperTest`, even though there's not really an opportunity for a mismatch there, given that there's only one alternative class loader. RELNOTES=`util.concurrent`: Changed the `guava-android` copy of `AbstractFuture` to try `VarHandle` before `Unsafe`, eliminating a warning under newer JDKs. PiperOrigin-RevId: 743198569
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is the next step now that we've migrated guava-jre off (#6806).
Some recent notes on Android: #6806 (comment)
Part of the work here is to have a fallback at all for a case or two that is missing one.
Then, ideally, we'd avoid even trying to use Unsafe in cases where it would produce a warning. But we don't want to regress performance on Android or ideally even the JVM.
One other small note: Currently, the Android flavor has no module-info. Today, if we were to give it one, we should really have a "full"
requiresline forjdk.unsupported. After this issue, though, we could mirror the JRE variant'sstaticline.The text was updated successfully, but these errors were encountered: