Skip to content
This repository has been archived by the owner on Nov 14, 2018. It is now read-only.

Bundle/PersistableBundle/ContentValues creation with Pairs #88

Closed
florina-muntenescu opened this issue Jan 15, 2018 · 2 comments
Closed

Comments

@florina-muntenescu
Copy link
Collaborator

Issue applies to Bundle, PersistableBundle and ContentValues but for simplicity the example will be given with Bundle.

Consider the following code:

Bundle().apply {
                putString(KEY_ANSWER_ONE, answerOne?.text?.toString())
                putString(KEY_ANSWER_TWO, answerTwo?.text?.toString())
 }

With the Android KTX API, this get transformed to:

bundleOf(Pair(KEY_ANSWER_ONE, answerOne?.text?.toString()),
         Pair(KEY_ANSWER_TWO, answerTwo?.text?.toString()))

There are two issues with this solution:

  1. We create a memory overhead by creating Pair objects, just to "transfer" the fields
  2. It gets easier to make mistakes and put in the Bundle an invalid object, due to the lack of type checks. In the above example, it's easy to put answerOne?.text instead of answerOne?.text?.toString(). If the object is serializable, we end up putting in the Bundle a different type (and possibly a bigger object) and we might have issues when reading the data from the Bundle. In case the object is not serializable, this will only be noticed at run time, when the app will crash.
@romainguy
Copy link
Collaborator

bundleOf() does not make Bundle().apply{} irrelevant. The extra allocations created by the Pair should not have a meaningful impact on apps unless they store thousands of items in bundles (which would indicate other issues). Short-lived allocations like this are also fairly well optimized out in N and O.

@JakeWharton
Copy link
Contributor

JakeWharton commented Jan 18, 2018

(Whoops I had a reply to this in a tab and forgot to hit enter.)

The full transformation with all syntactic niceties becomes:

bundleOf(
    KEY_ANSWER_ONE to answerOne?.text?.toString(),
    KEY_ANSWER_TWO to answerTwo?.text?.toString())

This syntax matches that of the various *Of factories in the stdlib which take pairs (e.g., mapOf, mutableMapOf, and our provided arrayMapOf).

One advantage to the allocation here is that the underlying collection can be pre-sized to exactly fit. This avoids it always starting empty, going through an allocation to 4 slots, and then having to double (based on how many elements you add).

Allocations like this wind up in many APIs. Hopefully someday the compiler will be able to see through the varargs and pair allocations someday. Or we could try getting the optimizations into D8, R8, or even ART.

The runtime check is the most unfortunate part, but as of yet there's no way to optimize it. We can bundle lint checks for it, though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants