Application Settings blocks UI thread

[ddfreiling]

Did you verify this is a real problem by searching Stack Overflow and the other open issues in this repo?

• YES

Tell us about the problem

Currently the application-settings module synchronize changes to disk every time a value is set. This could lead to main-thread blocking when setting large amounts of settings and/or large values. Especially at key moments, when 60fps is desired, like page transitions.

• Post iOS 7, one should not call synchronize after each change. (source)

  • Because this method is automatically invoked at periodic intervals, use this method only if you cannot wait for the automatic synchronization.

  • However it might be wise to do so on applicationDidEnterBackground, see source for reason.

• On Android, one should not call commit after each change, but instead apply. (source)

  • Commit blocks until changes are written to disk. Apply does so asynchronously and has been available since API 9. It is suggested to use this, if you do not care about the success boolean from commit.

Proposed Change

1. Do not synchronize on iOS and use apply on Android.
2. Add a flush call which synchronize/commit changes to disk, so that the user still has the option.
3. (Optional?) Add a call to appSettings.flush() on application.suspendEvent.

Side-note: Ideally I'd like to have something like RN's AsyncStorage

Plugin

@nota/nativescript-appsettings-async

To demo this I created a plugin with the changes proposed above. In the demo you can see the difference, which is quite big on Android. 4000ms /w commit vs. 100ms /w apply.

[vakrilov]

This seems like a reasonable request. The proposed options will introduce changes in behaviour though. I think it will be better to introduce a new set of async methods that will not block the UI thread.

Are you willing to open a PR with your implementation? You can check our contribution guidelines to get you started.

[ddfreiling]

@vakrilov Sure I don't mind submitting this as a PR.

The behavior change you are talking about would only be on iOS, if the app hard-exits almost immediately after a setX or remove. Android should be unchanged.

If you don't like the minor behavior change, would you then suggest adding these methods?

• asyncSetBoolean
• asyncSetNumber
• asyncSetString
• asyncRemove
• flush

[vakrilov]

I think it would be better to have Async as a suffix:

• setBooleanAsync
• setNumberAsync
• etc.

It is the more common convention (most notably in the the node APIs).

About the behaviour change - if your code sets a variable and than (almost)immediately reads it you might be affected. That's why I think new async methods a more safe bet.

[ddfreiling]

About the behaviour change - if your code sets a variable and than (almost)immediately reads it you might be affected. That's why I think new async methods a more safe bet.

Which platform does this happen on? As far as I know both iOS and Android has in-memory cache of NSUserDefaults and SharedPreferences respectively. So a read after change, would just access the in-memory cache.

Some quotes on this:

SharedPreferences.Editor.apply() commits its changes to the in-memory SharedPreferences immediately but starts an asynchronous commit to disk and you won't be notified of any failures.

NSUserDefaults caches the information to avoid having to open the user’s defaults database each time you need a default value. The synchronize method, which is automatically invoked at periodic intervals, keeps the in-memory cache in sync with a user’s defaults database.

[ddfreiling]

@vakrilov could you elaborate on the behaviour change, taking the above quotes into account?
Maybe I should just PR the change for android from commit to apply, since that is where the most performance gain was anyway, with no behaviour change.

[vakrilov]

Hey @ddfreiling - you are right, I wasn't aware about the in-memory cache.
Changing the the default implementation to apply instead of commit (Android) and omit the synchronize method (IOS) makes sense considering the information you just pointed.

Having an explicit flush method (2. form your original proposal) sounds reasonable too.

Thanks again for your involvement!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.