Http cache: First pass to move the HttpCache::Transaction to a sate machine....

Http cache: First pass to move the HttpCache::Transaction to a sate machine.

This CL only implements the states that are related to
current asynchronous operations.

BUG=26729
TEST=current unit tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33760

[rvargas (doing something else), 2009-11-20 00:12:41]

I'm trying to ease the review as much as possible so I'm not moving the code
around too much. I'll do a cleanup pass after the meat of the transition is in.

I'll also try to fix the PDF issue before checking this in to avoid a merging
mess :(

http://codereview.chromium.org/410005/diff/3001/4001
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/410005/diff/3001/4001#newcode551
Line 551: if (state < STATE_NONE || state >= STATE_LAST) {
I'm not actually convinced that we want to do this check at all. If we have a
corrupted state maybe the best thing is just to let it die.

I can remove this code and just let this method return void instead.

[eroman, 2009-11-20 00:43:16]

Awesome!

As a general comment, I am strongly in favor of making this into a state
machine; I think it will be a big win for readability, and ease of making
changes.

> I'm trying to ease the review as much as possible so I'm not moving the code
> around too much. I'll do a cleanup pass after the meat of the transition is
in.
> 
> I'll also try to fix the PDF issue before checking this in to avoid a merging
> mess :(

Sounds like a good plan.

http://codereview.chromium.org/410005/diff/3001/4001
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/410005/diff/3001/4001#newcode98
Line 98: struct HttpCache::Transaction::StateHandlerEntry {
I have a a comment about this in DoLoop()...

http://codereview.chromium.org/410005/diff/3001/4001#newcode498
Line 498: static const StateHandlerEntry StateHandlerTable[] = {
This setup of DoLoop() is very non-standard compared to our other state
machines.

While it is more verbose, I would really rather have this be a massive switch()
statement, modelled after http_network_transaction.cc.

I find the switch statement preferable as it is easier to understand, and easier
to extend if you want to insert trace statements.

http://codereview.chromium.org/410005/diff/3001/4001#newcode551
Line 551: if (state < STATE_NONE || state >= STATE_LAST) {
On 2009/11/20 00:12:42, rvargas wrote:
> I'm not actually convinced that we want to do this check at all. If we have a
> corrupted state maybe the best thing is just to let it die.
> 
> I can remove this code and just let this method return void instead.

I agree that CheckState() is clumsy, and recommend removing it... Reaching bad
states implies a logic problem in our statemachine, so might as well crash.

http://codereview.chromium.org/410005/diff/3001/4001#newcode778
Line 778: int HttpCache::Transaction::DoCacheQueryData(int result) {
The signature of these functions is awkward, since |result| is disregarded.

I reckon you only have the |result| parameter so it matches StateHandlerEntry
and can be an entry in the table.

I suggest replacing the table with a switch, and updating the function
signatures accordingly.

http://codereview.chromium.org/410005/diff/3001/4002
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/410005/diff/3001/4002#newcode97
Line 97: // Prototype of hanlder for the state machine.
typo

[rvargas (doing something else), 2009-11-20 01:44:18]

> This setup of DoLoop() is very non-standard compared to our other state
> machines.
> 
> While it is more verbose, I would really rather have this be a massive
switch()
> statement, modelled after http_network_transaction.cc.
> 
> I find the switch statement preferable as it is easier to understand, and
easier
> to extend if you want to insert trace statements.

Yes, I looked at the other implementations from net, and I even have an old
version that implements part of the common switch statement (expecting the
comment :)  )

However, I went with this approach because when the number of states is more
than just a few (as is the case for http_network_transaction), I find the switch
statement harder to look at... I would not be as easily sure that there is no
break statement missing, or if the trace is correct or if a dcheck is needed. I
kind of like the complete symmetry of having all the states with the same
signature and moving the dchecks from the switch to the actual state handling
code, thus making sure that the logic remains in the states and not in the
machine.

I agree that the switch is slightly easier to debug, but not by much (as the
code of DoLoop itself could be ignored, which doesn't happen with extra code on
the switch)... and the value of |state| provides almost the same debug
simplicity of a breakpoint on the switch...

As for adding trace or extra generic code, I think this is an intrinsic
advantage of the table because CheckState (for instance) could trivially
generate a custom message for every state without having to resort to writing 20
times the same few lines of code.

Did I convince you? :)

[eroman, 2009-11-20 02:19:53]

> Did I convince you? :)

I am not convinced it is worth the inconsistency; adding wtc for a second
opinion.

[darin (slow to review), 2009-11-20 05:20:16]

Ditto.  Moreover, if we want a different style of DoLoop, then we should
apply it universally.
The existing style, while verbose, is nice for setting breakpoints.
-Darin

On Thu, Nov 19, 2009 at 6:19 PM, <eroman@chromium.org> wrote:

> Did I convince you? :)
>>
>
> I am not convinced it is worth the inconsistency; adding wtc for a second
> opinion.
>
>
> http://codereview.chromium.org/410005
>

[rvargas (doing something else), 2009-11-20 23:10:44]

I would have volunteered to change the other implementations if consistency is
the only issue (although as I said, I see no issue if the switch is just a few
states).

Anyways, here's the other version.

On 2009/11/20 05:20:16, darin wrote:
> Ditto.  Moreover, if we want a different style of DoLoop, then we should
> apply it universally.
> The existing style, while verbose, is nice for setting breakpoints.
> -Darin
> 
> On Thu, Nov 19, 2009 at 6:19 PM, <mailto:eroman@chromium.org> wrote:
> 
> > Did I convince you? :)
> >>
> >
> > I am not convinced it is worth the inconsistency; adding wtc for a second
> > opinion.
> >
> >
> > http://codereview.chromium.org/410005
> >
>

[wtc, 2009-11-20 23:25:20]

I heard that an optimizing compiler will compile a
switch statement into a jump table like the one Ricardo
created manually if the values of the cases fall in a
consecutive range.  So I prefer a switch statement because
we avoid the risk of making a mistake when constructing a
jump table manually, rather than depending on the DCHECK_EQ
in CheckState().

I agree that all of our state machines should be implemented
in the same way.  If Ricardo comes up with an improvement,
we should apply it to the other state machines.

[rvargas (doing something else), 2009-11-21 00:07:09]

Yes, the compiler should optimize large switches to tables, but I don't consider
performance to be a guiding principle in this case for going one way or another.
I also dislike having to rely on a DCHECK, but it was the simplest solution that
also maintains the existence of the enum (if we are willing to avoid a separate
enumeration the table check can be automatic at compile time)... on the other
hand, hopefully we don't have any state machine with states not exercised by
unit tests :)

At the end, it's not that big of an issue, and if you guys are happy with the
switches I can certainly go with that.

On 2009/11/20 23:25:20, wtc wrote:
> I heard that an optimizing compiler will compile a
> switch statement into a jump table like the one Ricardo
> created manually if the values of the cases fall in a
> consecutive range.  So I prefer a switch statement because
> we avoid the risk of making a mistake when constructing a
> jump table manually, rather than depending on the DCHECK_EQ
> in CheckState().
> 
> I agree that all of our state machines should be implemented
> in the same way.  If Ricardo comes up with an improvement,
> we should apply it to the other state machines.

[eroman, 2009-11-21 02:45:07]

LGTM

http://codereview.chromium.org/410005/diff/3005/3006
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/410005/diff/3005/3006#newcode531
Line 531: HandleResult(rv);
This is non-standard with respect to our other DoLoop() functions -- we don't
call the callback from within DoLoop().

Rather, the callers of DoLoop() are responsible for checking rv !=
ERR_IO_PENDING.

Please consider matching that model (i am fine with making iterative changes).

http://codereview.chromium.org/410005/diff/3005/3006#newcode759
Line 759: DCHECK_EQ(OK, result);
This DCHECK seems to imply that DoCacheQueryData() can never fail?

I think we should be handling possible error results here, which might just
involve

if (result != OK)
  return result;

Iono.

http://codereview.chromium.org/410005/diff/3005/3006#newcode1319
Line 1319: DCHECK(result != ERR_IO_PENDING);
While u are here, could u change this to DCHECK_NE ?

http://codereview.chromium.org/410005/diff/3005/3007
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/410005/diff/3005/3007#newcode144
Line 144: int DoCacheWriteData(int result);
I think it is worth explaining what result is here, since it is somewhat
different then the other cases. (At a glance I expect this to be void).

[rvargas (doing something else), 2009-11-21 03:27:16]

Thanks. I hope I addressed all your comments.

http://codereview.chromium.org/410005/diff/3005/3006
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/410005/diff/3005/3006#newcode531
Line 531: HandleResult(rv);
On 2009/11/21 02:45:07, eroman wrote:
> This is non-standard with respect to our other DoLoop() functions -- we don't
> call the callback from within DoLoop().
> 
> Rather, the callers of DoLoop() are responsible for checking rv !=
> ERR_IO_PENDING.
> 
> Please consider matching that model (i am fine with making iterative changes).

Actually, I think that's a serious problem of the current code: it is impossible
to know if at a given point the result should be handled directly by a method,
or return hoping that at some point somebody will invoke the callback. I'm sure
this file will end up much better if we keep a simple and consistent pattern.

If at the end this doesn't make sense, I'll move this code from here and into
the relevant methods, but if it does, I could go though the other
implementations to see if the pattern also make sense there.

http://codereview.chromium.org/410005/diff/3005/3006#newcode759
Line 759: DCHECK_EQ(OK, result);
On 2009/11/21 02:45:07, eroman wrote:
> This DCHECK seems to imply that DoCacheQueryData() can never fail?
> 
> I think we should be handling possible error results here, which might just
> involve
> 
> if (result != OK)
>   return result;
> 
> Iono.

Yes, this callback cannot return anything else than OK because it only notifies
that an event has happened.

At the end of the refactor, this state should go away.

http://codereview.chromium.org/410005/diff/3005/3006#newcode1319
Line 1319: DCHECK(result != ERR_IO_PENDING);
On 2009/11/21 02:45:07, eroman wrote:
> While u are here, could u change this to DCHECK_NE ?

Actually I'll just remove it. This is guaranteed by the state machine and if we
have one we might as well dcheck on every state.

http://codereview.chromium.org/410005/diff/3005/3007
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/410005/diff/3005/3007#newcode144
Line 144: int DoCacheWriteData(int result);
On 2009/11/21 02:45:07, eroman wrote:
> I think it is worth explaining what result is here, since it is somewhat
> different then the other cases. (At a glance I expect this to be void).

This is exactly the same as the other methods... the result of the previous
state (the number of bytes read from the net). I can add the generic comment
from net_tx.h, but I felt that it wasn't really saying much. Is it better now?

nit: I would actually prefer if all the methods were the same... just receiving
the previous result and deciding by themselves if they want it or not. But I'll
shut up now :)

[eroman, 2009-11-21 04:12:56]

ok, lgtm.

[rvargas (doing something else), 2009-12-04 00:17:58]

New version to be commited:
The only real change (other than merging recent changes) is line 1345 of the cc
file, where I'm removing !callback_ from the conditions to avoid restarting the
request, because the SM is able to handle that case.

On 2009/11/21 04:12:56, eroman wrote:
> ok, lgtm.