IssueTracker

Background context:

• Bundle always returns an implicit default value for primitives but returns null for reference types.
• Performance is a top priority, and achieving a "zero-cost abstraction" has been a key goal for androidx.savedstate in its transition to KMP, as outlined in go/bundle-kmp (design doc).
• Requiring users to rely on the Bundle variant is not an option, as these APIs are designed specifically for KMP clients.

There are a few design options to address this problem if we consider the background context:

• Option 1: Prioritize API consistency while keeping the performance impact minimal -- but not zero.
  • Pros:
    • Ensures all APIs behave in a more type-safe and consistent manner.
  • Cons:
    • Increases inlined code from 2 to 11 LOC per method call.
    • Additional method calls and checks in the worst-case scenario.
    • Minimal impact in the best-case scenario.
  • Implementation:
    • This approach has been proposed by the reporter in aosp/3500556.
    • Note: The benchmark in aosp/3500556 is compared against the current implementation, not option 2.
• Option 2: Give a step back, and focus on a true "zero-cost abstraction."
  • Pros:
    • Matches Bundle behavior: familiar for Java and Android developers.
    • Inlined code reduced to 1 LOC.
    • There is zero performance runtime overhead.
  • Cons:
    • Matches Bundle behavior: (may be) unfamiliar for other platforms.
    • Inconsistency between primitives and reference getters.
      • Note: null values would result in an Exception as we never return nullable types in SavedState instead.
  • Implementation:
    • Ensure all methods directly call Bundle.
    • Remove any additional checks, including keyNotFoundError. They (probably) shouldn't have been added, as they violate zero-cost abstraction.

We need to finalize a design decision before moving forward and document the outcome here.

Thanks for the summary and background. I think we can also add option 3: combine 1 and 2 and provide, say:

• getXXX() // closest to Bundle
• getXXXOrThrow() // behaves like getXXX() in current implementation
• getXXXOrElse()

That "zero-cost abstraction" in the design doc reads more like it's referring to the types SavedStateReader/SavedStateWriter and less so for their functions. I consider our goal here is to make a "better" Bundle, and that probably includes (but not limited to):

1. cross platform
2. more Kotlin idiomatic (e.g. no null, naming closer to Kotlin's Map, etc.)
3. safer (e.g. extra checks to report mistakes)
4. more sensible (e.g. List instead of ArrayList, more consistent, etc.)

Also some notes on "Minimal impact in the best-case scenario." for option 1. It's actually more like some impacts in different scenarios. Some good, some bad. Here are some ballpark numbers for getXXX:

1. Bundle - 20ns

2. current implementation:

• has key
  • has valid value - 40ns
  • has no valid value - 40ns (return default value)
• has no key - 20ns (throw)

3. aosp/3500556:

• has key
  • has valid value
    • value is equal to default - 40ns
    • value is not equal to default - 20ns
  • has no valid value - 60ns (throw)
• has no key - 60ns (throw)

Bolded situations are what I think the most likely situations.

That "zero-cost abstraction" in the design doc reads more like it's referring to the types SavedStateReader/SavedStateWriter and less so for their functions.

To clarify, when I wrote that sentence, my focus was on:

• Avoiding unnecessary allocations (compared to Bundle).
• Avoiding unnecessary work (compared to Bundle).
• Minimizing inlined code (compared to Bundle).

I consider our goal here is to make a "better" Bundle, and that probably includes (but not limited to).

That's correct, and the goal also includes a low/zero-cost abstraction while maintaining backward compatibility with existing APIs.

About option 3, I would avoid this path as that means increasing the Public API surface.

Let's discuss offline tomorrow in our sync and document the outcome here.

Adding more numbers. Testing the happy path (bundle has value of correct type) using Microbenchmark on a Pixel Fold with a small Bundle with 6 entries.

The candidats are:

• Bundle.getXXX - the overload without default value
• Bundle.getXXX(with default) - the overload with default value
• current - current implementation
• no collision - aosp/3500738 when we only need to call getXXX once
• collision - aosp/3500738 when we need to call getXXX twice
• no collision, no inline - aosp/3500738 when we only need to call getXXX once, but remove the inline
• collision, no inline - aosp/3500738 when we need to call getXXX twice, but remove the inline

Here's the result:

Bundle.getXXX without default value is surprisingly slow. And inlining has a slight performance benefit.

Project: platform/frameworks/support
Branch: androidx-main
Author: Kuan-Ying Chou <kuanyingchou@google.com>
Link:      https://android-review.googlesource.com/3500556

Make the rest SavedStateReader.getXXX() functions throw when value type doesn't match return type

Make the rest SavedStateReader.getXXX() functions throw when value type doesn't match return type 
 
We throw IllegalStateException when the saved value's type does't match function's return type in most of our `SavedStateReader.getXXX()` functions, but return a pre-defined default value in the following functions on Android: 
 
- getBoolean 
- getChar 
- getDouble 
- getFloat 
- getInt 
- getLong 
 
In this CL we make these throw as well so the behaviors for all `getXXX()` are consistent. If a default value is preferred users should use the `getXXXOrElse` variants. 
 
To achieve this on Android without using deprecated APIs we call the corresponding functions in `Bundle` twice when it's possible that the value does not have a correct type. With some optimization it's actually faster than the current version in most cases. 
 
Here's my test on a Pixel Fold using Microbenchmark: 
 
              current    this CL(no collision)   this CL(collision) 
getBoolean    43.1 ns                 22.7 ns               44.5 ns 
getChar       44.8 ns                 23.6 ns               46.0 ns 
getDouble     42.8 ns                 22.7 ns               45.8 ns 
getFloat      43.1 ns                 23.0 ns               46.0 ns 
getInt        43.4 ns                 22.9 ns               44.4 ns 
getLong       43.9 ns                 23.9 ns               45.2 ns 
 
"Collision" here means the saved value is the same as our chosen default value (when we need to call `getXXX` twice). This should be like 50% of time for Boolean and less likely for other types. There is a minor slowdown (~3% to 7%) when collision happens, but ~45% to 47% improvement otherwise. 
 
Test: SavedStateTest 
Bug: 399317598 
Relnote: Make the rest SavedStateReader.getXXX() functions throw when value type doesn't match return type 
Change-Id: I78c4adedf59bc93ae8851ef105b7ef362bd37a64