1240397 - Better use of StringBuilder in GeckoProfile

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Profile Handling, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: geckoprofile-init-sb v0.1

We read files, JSON session files usually, using a StringBuilder but we but initialize the StringBuilder to the file size. This means the buffer needs to reallocate and copy as it grows.

Passing a size to the constructor helps reduce GC churn.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a78578829c&selectedJob=15546634

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8708833 [details] [diff] [review]
geckoprofile-init-sb v0.1

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

::: mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java
@@ +678,5 @@
>  
>      private String readFile(File target) throws IOException {
>          FileReader fr = new FileReader(target);
>          try {
> +            StringBuilder sb = new StringBuilder((int)target.length() + 16);

This stats the file.

http://androidxref.com/4.4.4_r1/xref/libcore/luni/src/main/java/java/io/File.java#727

That I/O step will almost certainly cost more than reallocating the StringBuilder buffer, even if we're about to read it anyway.

Can we just figure out a reasonable static upper bound here, or allow callers to pass in such a bound?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #2)
> Comment on attachment 8708833 [details] [diff] [review]
> geckoprofile-init-sb v0.1
> 
> Review of attachment 8708833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java
> @@ +678,5 @@
> >  
> >      private String readFile(File target) throws IOException {
> >          FileReader fr = new FileReader(target);
> >          try {
> > +            StringBuilder sb = new StringBuilder((int)target.length() + 16);
> 
> This stats the file.
> 
> http://androidxref.com/4.4.4_r1/xref/libcore/luni/src/main/java/java/io/File.
> java#727
> 
> That I/O step will almost certainly cost more than reallocating the
> StringBuilder buffer, even if we're about to read it anyway.

Really? We are about to read the file anyway and the StringBuilder affects are memory related, not I/O related.

> Can we just figure out a reasonable static upper bound here, or allow
> callers to pass in such a bound?

I can pass the same 8KB number passed to the buffer.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #3)

> Really? We are about to read the file anyway and the StringBuilder affects
> are memory related, not I/O related.

Two reasons why it's expensive:

* stat is a kernel call, so we'd be leaving user space, touching the filesystem (if it's not already cached), then returning to user space to handle the result. Yes, we might be about to read the file anyway -- so the inode will be cached for the later read -- but the stat itself is still relatively expensive. 

* Calls into Java's File APIs aren't themselves free. I haven't measured, but some cursory research indicates that they do allocate (e.g., the filename itself needs memory to be allocated fresh). That might even outweigh the initial size of the SB, making it cheaper to eat the resize for files smaller than 8KB!

Only measurement will say whether the additional cost of the call (in memory and wall clock) is worthwhile compared to the cost of resizing the StringBuilder. Without those measurements at various file sizes we're speculating blindly.


> > Can we just figure out a reasonable static upper bound here, or allow
> > callers to pass in such a bound?
> 
> I can pass the same 8KB number passed to the buffer.

It looks like we mostly use this for reading session store and the tab queue, so 8KB seems like a reasonable starting point.

If you want to get slightly more perf-conscious -- and it seems that you do! -- note that you can delay allocation of the StringBuilder until *after* the first read into the buffer. At that point you know the exact size (if it's smaller than the buffer size), so you can choose whether to start at that, or at 2 * the buffer size.

Comment 5

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.