Batch operations for nsCookieService

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking: Cookies
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: junior, Assigned: junior)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 months ago
Per bug 1158387, AsyncReadComplete, EnsureReadDomain, EnsureReadComplete and RemoveCookiesWithOriginAttributes use multiple transactions. It would jank if synchronous = NORMAL
(Assignee)

Comment 1

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de126b6962b30dd259a44a0f852080272dda734a
(Assignee)

Comment 2

5 months ago
Created attachment 8875113 [details] [diff] [review]
batchOperation - v1
Attachment #8875113 - Flags: review?(jduell.mcbugs)
Attachment #8875113 - Flags: feedback?(mak77)

Comment 3

5 months ago
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.
Attachment #8875113 - Flags: feedback?(mak77)
(Assignee)

Comment 4

5 months ago
(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

5 months ago
(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 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)
Attachment #8875113 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 7

5 months ago
Created attachment 8875662 [details] [diff] [review]
batchOperation - v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a70fa483f9195f4b8a617c2bb71022f9fa121408
Attachment #8875113 - Attachment is obsolete: true
Attachment #8875662 - Flags: review+
Attachment #8875662 - Flags: feedback?(mak77)

Comment 8

5 months ago
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.
Attachment #8875662 - Flags: feedback?(mak77) → feedback+
(Assignee)

Comment 9

5 months ago
Created attachment 8875974 [details] [diff] [review]
batchOperation - v3

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a7eb9b093785d6bbf51652363d78bf9282e7b9a
Attachment #8875662 - Attachment is obsolete: true
Attachment #8875974 - Flags: review+
Attachment #8875974 - Flags: feedback+
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 10

5 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3eb05ddb04
Batch operations for nsCookieService, r=jduell
Keywords: checkin-needed

Comment 11

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eb3eb05ddb04
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.