1370456 - Batch operations for nsCookieService

Status: RESOLVED FIXED

(Core :: Networking: Cookies, enhancement)

Description

[Junior [inactive]]

Per bug 1158387, AsyncReadComplete, EnsureReadDomain, EnsureReadComplete and RemoveCookiesWithOriginAttributes use multiple transactions. It would jank if synchronous = NORMAL

Comment 1

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de126b6962b30dd259a44a0f852080272dda734a

Comment 2

[Junior [inactive]]

Attachment: batchOperation - v1

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8875113 [details] [diff] [review]
batchOperation - v1

Review of attachment 8875113 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know this code well to tell whether this covers all the cases, thus I'm just looking at the helper usage.
Are all these cases commonly invoked with arrays bigger than one entry? since there is some overhead for wrapping a single write in a transaction.

::: netwerk/cookie/nsCookieService.cpp
@@ +2742,5 @@
>    NS_ASSERTION(mDefaultDBState, "no default DBState");
>    NS_ASSERTION(mDefaultDBState->pendingRead, "no pending read");
>    NS_ASSERTION(mDefaultDBState->readListener, "no read listener");
>  
> +  mozStorageTransaction transaction(mDefaultDBState->dbConn, true);

the second argument should be false if you plan to Commit manually, and that makes more sense. The idea is that if someone in the future throws in the middle we will rollback.

@@ +4921,5 @@
>      NS_WARNING("No DBState! Profile already close?");
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> +  mozStorageTransaction transaction(mDBState->dbConn, true);

I'd suggest to use the helper in a coherent way, so either always autocommit, or never and manually Commit(). The latter in general is a bit safer to future code changes.

Comment 4

[Junior [inactive]]

(In reply to Marco Bonardo [::mak] from comment #3)
> Comment on attachment 8875113 [details] [diff] [review]
> batchOperation - v1
> 
> Review of attachment 8875113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know this code well to tell whether this covers all the cases, thus
> I'm just looking at the helper usage.
> Are all these cases commonly invoked with arrays bigger than one entry?
> since there is some overhead for wrapping a single write in a transaction.

That's what I concerned. It may be zero to many transaction in these place
but I couldn't know the fact before those for-loop.

On the other hand, these usage are usually with many row-operations, which is good.

Therefore, I guess we could keep the wrapper.
If the overhead matters, maybe we should use ParamsArray instead of mozStorageTransaction.

> 
> ::: netwerk/cookie/nsCookieService.cpp
> @@ +2742,5 @@
> >    NS_ASSERTION(mDefaultDBState, "no default DBState");
> >    NS_ASSERTION(mDefaultDBState->pendingRead, "no pending read");
> >    NS_ASSERTION(mDefaultDBState->readListener, "no read listener");
> >  
> > +  mozStorageTransaction transaction(mDefaultDBState->dbConn, true);
> 
> the second argument should be false if you plan to Commit manually, and that
> makes more sense. The idea is that if someone in the future throws in the
> middle we will rollback.
> 
> @@ +4921,5 @@
> >      NS_WARNING("No DBState! Profile already close?");
> >      return NS_ERROR_NOT_AVAILABLE;
> >    }
> >  
> > +  mozStorageTransaction transaction(mDBState->dbConn, true);
> 
> I'd suggest to use the helper in a coherent way, so either always
> autocommit, or never and manually Commit(). The latter in general is a bit
> safer to future code changes.

I'd like to confirm if your suggestion is:

mozStorageTransaction transaction(aConnection, false);

// do some transactions
 
aConnection->Commit();

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Junior[:junior] from comment #4)
> I'd like to confirm if your suggestion is:
> 
> mozStorageTransaction transaction(aConnection, false);
> 
> // do some transactions
>  
> aConnection->Commit();

yes

Comment 6

[Jason Duell]

Comment on attachment 8875113 [details] [diff] [review]
batchOperation - v1

Review of attachment 8875113 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cookie/nsCookieService.cpp
@@ +2882,5 @@
>      array.AppendElement(GetCookieFromRow(mDefaultDBState->stmtReadDomain,
>                                           aKey.mOriginAttributes));
>    }
>  
> +  mozStorageTransaction transaction(mDefaultDBState->dbConn, true);

Like Marco said, all of these transactions should have 'false' for the 2nd parameter in the constructor (and then we do a manual Commit() call)

Comment 7

[Junior [inactive]]

Attachment: batchOperation - v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a70fa483f9195f4b8a617c2bb71022f9fa121408

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8875662 [details] [diff] [review]
batchOperation - v2

Review of attachment 8875662 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cookie/nsCookieService.cpp
@@ +2757,5 @@
>        continue;
>  
>      AddCookieToList(tuple.key, tuple.cookie, mDefaultDBState, nullptr, false);
>    }
> +  transaction.Commit();

nit: if you don't care about an eventual error you may "mozilla::Unused << ". Or you could use a "DebugOnly<nsresult> rv = " (if you don't have an rv var around already) and MOZ_ASSERT(NS_SUCCEEDED(rv));
In general it shouldn't happen, but due to unexpected db conditions you may hit one of these in the future and you'll be happy to have them.

Comment 9

[Junior [inactive]]

Attachment: batchOperation - v3

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a7eb9b093785d6bbf51652363d78bf9282e7b9a

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3eb05ddb04
Batch operations for nsCookieService, r=jduell

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb3eb05ddb04