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

Add ability to specify durability for transactions in Chrome #1367

Merged
merged 9 commits into from Aug 2, 2021
Merged

Add ability to specify durability for transactions in Chrome #1367

merged 9 commits into from Aug 2, 2021

Conversation

alyssa-glean
Copy link
Contributor

@alyssa-glean alyssa-glean commented Jul 23, 2021

Implementing step 1 as described in this issue

This PR updates dexie so that we can pass a chromeTransactionDurability when constructing a Dexie instance to drill down into any transactions that are used when interacting with it. This is a special flag that was added by Chrome per this feature (source code for the change is here). For other browsers (or versions of Chrome older than 83) it will have no impact.

I've added tests to check that the raw IDBTransaction has the desired state based on what is passed, and have also used the npm link strategy to verify that there's a performance boost to our app when using the modified Dexie code. We wrote a simple test that inserted blobs into a table in a loop, under three scenarios:

  1. Using Dexie.Table
  2. Using raw IDBTransaction with { durability: 'relaxed' }
  3. Using raw IDBTransaction with { durability: 'default' }

Before the change, the timings for 1) and 3) aligned, with 2) being about twice as fast. Linking the changes in this PR, the timings for 1) reduced in-line with 2). Happy to share the source code for the test we ran if it's of any interest.

@dfahlander
Copy link
Collaborator

dfahlander commented Jul 23, 2021

Thanks! I couldn't get the same performance benefits when I tested this out earlier. Probably depends a lot about the use case. Not sure that changing the default is ok in a minor version so for 3.x we could allow the option in Dexie constructor but keep the default durability and plan to change the default for next major (4.x).

@alyssa-glean
Copy link
Contributor Author

alyssa-glean commented Jul 23, 2021

Thanks! I couldn't get the same performance benefits when I tested this out earlier. Probably depends a lot about the use case. Not sure that changing the default is ok in a minor version so for 3.x we could allow the option in Dexie constructor but keep the default durability and plan to change the default for next major (4.x).

Yeah that makes sense - I guess I was hoping to get away with the lazy option 😛

I'll look at plumbing the durability properly so it's configurable. Also, looks like tests are failing because they get run on multiple browsers (so my new tests fails on Firefox for instance - see here). Is there an established pattern for having tests run only on certain browsers that I can follow to get this passing?

Alex added 2 commits Jul 23, 2021
@alyssa-glean
Copy link
Contributor Author

alyssa-glean commented Jul 23, 2021

Updated to plumb as a DexieOption - also renamed to chromeTransactionDurability so it's super clear it's a Chrome-specific thing.

I also extracted a separate test file for this stuff since I need to construct a Dexie db per test with different options. Still got the outstanding question of how to skip these tests for non-chrome so we can get the Travis build to pass, otherwise I think this is ready for review/release in a minor 3.x version 🤞

@dfahlander
Copy link
Collaborator

dfahlander commented Jul 23, 2021

Great! There is a supports() function in dexie-unittest-itils.js that you could extend. It's also ok to do user-agent sniffing here. If the browser doesn't support the feature, let the test succeed with ok(true, description).

@alyssa-glean
Copy link
Contributor Author

alyssa-glean commented Jul 23, 2021

Ah nice! I've put in an isChrome var which seems to work for me. It detects newer versions of Edge, too - but that's fine since it's basically the same browser these days (it's just reskinned chrome, so the tests will pass there I imagine).

@alyssa-glean
Copy link
Contributor Author

alyssa-glean commented Aug 1, 2021

Is there anything else you need from me on this? Would be awesome to get it merged and released if possible so we can see the impact it has on our users (if this doesn't provide a sufficient enough performance boost, the sooner we find out the sooner we can do more investigation / look into other avenues) 🤞

@dfahlander
Copy link
Collaborator

dfahlander commented Aug 1, 2021

Currently it's just the comment I made about the variable isChrome not working (which basically makes it not trying your tests on chrome either).

@dfahlander
Copy link
Collaborator

dfahlander commented Aug 1, 2021

I thought I commented with a jsfiddle link but can't find it now. Anyway, I've tried the isChrome variable in a fiddle and it evaluated to false on all browsers.

@alyssa-glean
Copy link
Contributor Author

alyssa-glean commented Aug 2, 2021

Ah interesting - yeah, for me locally in chrome it was skipping the tests too it turns out. It's odd - I tested the flag using chrome dev tools and it worked in there 🤷

I've simplified the flag and it seems to be working now 👍

@alyssa-glean alyssa-glean changed the title Use relaxed durability for transactions by default Add ability to specify durability for transactions in Chrome Aug 2, 2021
@dfahlander dfahlander merged commit f42e3cc into dexie:master Aug 2, 2021
1 check passed
@dfahlander
Copy link
Collaborator

dfahlander commented Aug 2, 2021

Thanks! It's merged.

@alyssa-glean alyssa-glean deleted the relaxed-durability branch Aug 2, 2021
@alyssa-glean
Copy link
Contributor Author

alyssa-glean commented Aug 2, 2021

Thanks! It's merged.

Fantastic! Thanks for your help and super speedy responses - this has been an excellent contribution experience from my perspective 😁

Any idea when a new release might be published? Excited to get this out into the wild so we can see how it affects our metrics in production 🤤

@dfahlander
Copy link
Collaborator

dfahlander commented Aug 2, 2021

Happy to hear that 👍
I will start the release-script this afternoon. It does some additional tests on complementary browsers but unless any script fails, it would be out soon.

@dfahlander
Copy link
Collaborator

dfahlander commented Aug 2, 2021

Sorry, the release-script failed on a regression test on Chrome 79 (listed in our config file as "chrome_oldest_supported"). Obviously, it's a chrome but probably without the support for this option. Either, we could update the chrome_oldest_supported to a version that has support for this option, or we could extend the test condition to also verify chrome version (could be extracted using a regexp on navigator.userAgent). Another option would be to allow undefined as a valid value of trans.idbtrans.durability (as it would only be set to "default" or "relaxed" if it's a chrome that supports it)

@dfahlander
Copy link
Collaborator

dfahlander commented Aug 2, 2021

Is it ok if I fix it by allowing trans.idbtrans.durability to be undefined in all three tests?

@alyssa-glean
Copy link
Contributor Author

alyssa-glean commented Aug 2, 2021

Is it ok if I fix it by allowing trans.idbtrans.durability to be undefined in all three tests?

Yep, sounds good!

@dfahlander
Copy link
Collaborator

dfahlander commented Aug 2, 2021

It's released. If you'd get inspiration to update the docs for Dexie constructor (https://dexie.org/docs/Dexie/Dexie) that would be a nice thing. If not, that's ok also and we try make sure fixing it later. The current doc is stone-age old with clumsy HTML table. It's ok to update it to a markdown table if you have that inspiration. (Use the "Edit on github" button to edit the page and it will generate a PR)

@alyssa-glean
Copy link
Contributor Author

alyssa-glean commented Aug 2, 2021

It's released. If you'd get inspiration to update the docs for Dexie constructor (https://dexie.org/docs/Dexie/Dexie) that would be a nice thing. If not, that's ok also and we try make sure fixing it later. The current doc is stone-age old with clumsy HTML table. It's ok to update it to a markdown table if you have that inspiration. (Use the "Edit on github" button to edit the page and it will generate a PR)

Thanks so much! Updating some docs feels like the least I can do - I'll aim to get to it at some point this afternoon 😄

alyssa-glean added a commit to alyssa-glean/dexie.js-web that referenced this issue Aug 2, 2021
Also updates to use markdown tables rather than HTML, per the suggestion [here](dexie/Dexie.js#1367 (comment))
dfahlander pushed a commit to dfahlander/dexie.js-web that referenced this issue Aug 2, 2021
Also updates to use markdown tables rather than HTML, per the suggestion [here](dexie/Dexie.js#1367 (comment))
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.

None yet

2 participants