[MD settings] stream i18n replacement without large accumulation buffer

[MD settings] stream i18n replacement without large accumulation buffer

This CL processes the i18n source stream data in smaller pieces so that
the data is not accumulated (buffered). This avoids the potential for
the i18n streaming use too much memory.

BUG=677555
Committed: https://crrev.com/3ba7a97bb01f0838d0d994f768a8af2f931afa16
Cr-Commit-Position: refs/heads/master@{#441241}

[dschuyler, 2016-12-29 21:49:39]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 21:50:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-29 22:13:59]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 22:14:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-29 22:39:58]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 22:40:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-29 22:50:07]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 22:50:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-29 22:50:38]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-12-29 22:50:47]

Patchset #1 (id:20001) has been deleted

[dschuyler, 2016-12-29 22:59:26]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 22:59:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-29 23:03:35]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 23:03:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-29 23:04:17]

Patchset #1 (id:40001) has been deleted

[dschuyler, 2016-12-29 23:04:25]

Patchset #1 (id:60001) has been deleted

[dschuyler, 2016-12-29 23:24:26]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 23:24:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-29 23:24:56]

Patchset #2 (id:100001) has been deleted

[dschuyler, 2016-12-29 23:27:26]

dschuyler@chromium.org changed reviewers:
+ mmenke@chromium.org

[dschuyler, 2016-12-29 23:27:26]

The two patches (#1 and #2) are alternate versions of this CL. We can go with
either. 

Patch #1 has fewer conditions making the code easier to read with small
sacrifices in time and space. The time cost is less than 1 msec and the space
difference may be KB (should not MB).

Patch #2 is more complex and slightly faster (with a stronger guarantee of less
memory being used). Though the complexity only buys us less than 1 msec (on a
specialist dev machine; so this may be 1 msec+ on a low end machine) and < a MB.

I'm ready to go with Patch #2.

[commit-bot: I haz the power, 2016-12-30 00:21:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-30 00:21:04]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-03 16:47:16]

I prefer the first one - second seems overkill, and too closely mirrors internal
code of I18nSourceStream::FilterData.

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:48: size_t pos =
input_.find_last_of("$}");
optional:  Maybe toss in "\r\n"?  Not sure how common $'s are that aren't part
of $i18n's are, but may make sense to handle them a bit better. 
I18nSourceStream::FilterData doesn't explicitly disallow \r or \n in names, but
it assumes keys are shorter than a line, which seems to imply they don't include
CRs/LFs.

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:50: if (!upstream_end_reached && pos
!= std::string::npos && input_[pos] == '$') {
A string ending in "$" now has special case code.  Should have a test for it.

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:54: input_.assign(input_, pos,
std::string::npos);
input_.erase(0, pos);?  Should do the same thing, but may open up more space for
compiler optimizations, and while std::string self-assignment may work this way,
it seems a bit ugly.

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:58: input_.clear();
to_process.swap(input_);  accomplishes what both these lines do without the
additional copy.  If also has the advantage of actually "freeing" the memory
owned by input_, since clear() generally doesn't free memory.

Alternatively, should make to_process a StringPiece, and have "input_.erase(0,
to_process.size());" after the output_.append call (i.e., just have a shared
method to truncate append on both paths).

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:65: output_.assign(output_,
bytes_out, std::string::npos);
ouput_.erase(0, bytes_out);

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
File content/browser/webui/i18n_source_stream.h (right):

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:43: // Keep split $i18n tags until we
have the whole tag. This is expected to vary
nit:  Should avoid "we" in comments, since it's often not clear who "we" is,
particularly for non-native speakers.

[dschuyler, 2017-01-03 20:23:31]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 20:23:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-01-03 20:37:34]

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:48: size_t pos =
input_.find_last_of("$}");
On 2017/01/03 16:47:16, mmenke wrote:
> optional:  Maybe toss in "\r\n"?  Not sure how common $'s are that aren't part
> of $i18n's are, but may make sense to handle them a bit better. 
> I18nSourceStream::FilterData doesn't explicitly disallow \r or \n in names,
but
> it assumes keys are shorter than a line, which seems to imply they don't
include
> CRs/LFs.

Done.

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:50: if (!upstream_end_reached && pos
!= std::string::npos && input_[pos] == '$') {
On 2017/01/03 16:47:16, mmenke wrote:
> A string ending in "$" now has special case code.  Should have a test for it.

Done.

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:54: input_.assign(input_, pos,
std::string::npos);
On 2017/01/03 16:47:16, mmenke wrote:
> input_.erase(0, pos);?  Should do the same thing, but may open up more space
for
> compiler optimizations, and while std::string self-assignment may work this
way,
> it seems a bit ugly.

Done.

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:58: input_.clear();
On 2017/01/03 16:47:16, mmenke wrote:
> to_process.swap(input_);  accomplishes what both these lines do without the
> additional copy.  If also has the advantage of actually "freeing" the memory
> owned by input_, since clear() generally doesn't free memory.
> 
> Alternatively, should make to_process a StringPiece, and have "input_.erase(0,
> to_process.size());" after the output_.append call (i.e., just have a shared
> method to truncate append on both paths).

Done.

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.cc:65: output_.assign(output_,
bytes_out, std::string::npos);
On 2017/01/03 16:47:16, mmenke wrote:
> ouput_.erase(0, bytes_out);

Done.

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
File content/browser/webui/i18n_source_stream.h (right):

https://codereview.chromium.org/2601313002/diff/80001/content/browser/webui/i...
content/browser/webui/i18n_source_stream.h:43: // Keep split $i18n tags until we
have the whole tag. This is expected to vary
On 2017/01/03 16:47:16, mmenke wrote:
> nit:  Should avoid "we" in comments, since it's often not clear who "we" is,
> particularly for non-native speakers.

Done.

[mmenke, 2017-01-03 20:47:32]

LGTM!

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:51: size_t pos =
input_.find_last_of("$} \t\f\v\r\n");
Are spaces and tabs not allowed in string identifiers?  I wasn't sure.

Don't think we need \v and \f - they're so rare that including them seems
unnecessary.

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
File content/browser/webui/i18n_source_stream_unittest.cc (right):

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
content/browser/webui/i18n_source_stream_unittest.cc:223: const char
source_data[] = "test with tag at end of line $";
nit:  kSourceData

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
content/browser/webui/i18n_source_stream_unittest.cc:226:
source()->AddReadResult(source_data + source_size, 0, net::OK,
Know it's what the other tests do, but the first argument here really should
just be nullptr.  Feel free to update the other tests, or not.

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
content/browser/webui/i18n_source_stream_unittest.cc:231:
EXPECT_EQ(std::string(source_data, source_size), actual_output);
std::string(source_data, source_size) -> source_data.  It will be implicitly
converted to an std::string.  Again, can optionally update other tests
similarly.

[commit-bot: I haz the power, 2017-01-03 21:20:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 21:20:35]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-01-03 21:50:49]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 21:51:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-01-03 21:51:40]

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
File content/browser/webui/i18n_source_stream.cc (right):

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
content/browser/webui/i18n_source_stream.cc:51: size_t pos =
input_.find_last_of("$} \t\f\v\r\n");
On 2017/01/03 20:47:32, mmenke wrote:
> Are spaces and tabs not allowed in string identifiers?  I wasn't sure.
> 
> Don't think we need \v and \f - they're so rare that including them seems
> unnecessary.

Ah, I wondered if omitting them would beg the question of why they were omitted
(I'm used to seeing them in the list of white-space). I agree that they are rare
and it's an optimization so they are not necessary.

Done.

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
File content/browser/webui/i18n_source_stream_unittest.cc (right):

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
content/browser/webui/i18n_source_stream_unittest.cc:223: const char
source_data[] = "test with tag at end of line $";
On 2017/01/03 20:47:32, mmenke wrote:
> nit:  kSourceData

Done.

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
content/browser/webui/i18n_source_stream_unittest.cc:226:
source()->AddReadResult(source_data + source_size, 0, net::OK,
On 2017/01/03 20:47:32, mmenke wrote:
> Know it's what the other tests do, but the first argument here really should
> just be nullptr.  Feel free to update the other tests, or not.

Done.

https://codereview.chromium.org/2601313002/diff/140001/content/browser/webui/...
content/browser/webui/i18n_source_stream_unittest.cc:231:
EXPECT_EQ(std::string(source_data, source_size), actual_output);
On 2017/01/03 20:47:32, mmenke wrote:
> std::string(source_data, source_size) -> source_data.  It will be implicitly
> converted to an std::string.  Again, can optionally update other tests
> similarly.

Done.

[mmenke, 2017-01-03 21:53:55]

Thanks for doing this!  (And still LGTM)

[Dan Beam, 2017-01-03 22:06:56]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2017-01-03 22:06:56]

lgtm

[commit-bot: I haz the power, 2017-01-03 23:06:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 23:06:05]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-01-03 23:06:25]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-01-03 23:07:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 23:11:48]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1483484785194380,
"parent_rev": "005903e3a6d7e2dad8089281d38e5c185b349e08", "commit_rev":
"4135f4f05c163872d3ec68f9e471c6bca0dfc35b"}

[commit-bot: I haz the power, 2017-01-03 23:12:13]

Description was changed from

==========
[MD settings] stream i18n replacement without large accumulation buffer

This CL processes the i18n source stream data in smaller pieces so that
the data is not accumulated (buffered). This avoids the potential for
the i18n streaming use too much memory.

BUG=677555
==========

to

==========
[MD settings] stream i18n replacement without large accumulation buffer

This CL processes the i18n source stream data in smaller pieces so that
the data is not accumulated (buffered). This avoids the potential for
the i18n streaming use too much memory.

BUG=677555

Review-Url: https://codereview.chromium.org/2601313002
==========

[commit-bot: I haz the power, 2017-01-03 23:12:15]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2017-01-03 23:16:08]

Description was changed from

==========
[MD settings] stream i18n replacement without large accumulation buffer

This CL processes the i18n source stream data in smaller pieces so that
the data is not accumulated (buffered). This avoids the potential for
the i18n streaming use too much memory.

BUG=677555

Review-Url: https://codereview.chromium.org/2601313002
==========

to

==========
[MD settings] stream i18n replacement without large accumulation buffer

This CL processes the i18n source stream data in smaller pieces so that
the data is not accumulated (buffered). This avoids the potential for
the i18n streaming use too much memory.

BUG=677555

Committed: https://crrev.com/3ba7a97bb01f0838d0d994f768a8af2f931afa16
Cr-Commit-Position: refs/heads/master@{#441241}
==========

[commit-bot: I haz the power, 2017-01-03 23:16:09]

Patchset 4 (id:??) landed as
https://crrev.com/3ba7a97bb01f0838d0d994f768a8af2f931afa16
Cr-Commit-Position: refs/heads/master@{#441241}