Status

()

defect
RESOLVED DUPLICATE of bug 905227
7 years ago
6 years ago

People

(Reporter: mwu, Assigned: bjacob)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open])

Attachments

(2 attachments, 2 obsolete attachments)

Posted patch Enable Skia on B2G (obsolete) — Splinter Review
B2G is close enough to Android that this works largely out of the box.

cjones, what do you think?
Attachment #627421 - Flags: review?(jones.chris.g)
This will strictly make your binary bigger, fwiw. We don't use Skia for anything except plugins on Android right now.
Well when bug 725119 is landed then.
Depends on: skia-android
Comment on attachment 627421 [details] [diff] [review]
Enable Skia on B2G

s/Enable Skia/Enable building Skia/
Attachment #627421 - Flags: review?(jones.chris.g) → review+
bjacob told me that he thinks this is already enabled.  George, can you confirm?
We shouldn't bother enabling Skia on B2G unless we have a reason. It's slower on many things on Android.
(In reply to Andrew Overholt [:overholt] from comment #4)
> bjacob told me that he thinks this is already enabled.  George, can you
> confirm?

This isn't enabled AFAICT.
It shouldn't be enabled, as far as I know.
Added skia configuration on b2g but didn't turn on skia by default.
Attachment #772616 - Flags: review?(gwright)
Attachment #772616 - Flags: review?(gwright) → review+
Rebase to latest inbound
Attachment #772616 - Attachment is obsolete: true
Attachment #780924 - Flags: review+
Peter, can you back out? We weren't planning to build skia until we actually start using it.
How far away are we from turning on skia on mobile?
Does it cause problems to build it even before we start using it?
No, but we previously agreed not to turn it on until it would actually be used. No need to increase our codesize until we need it.
I am still much more interested in when we will start using this.
I was going to suggest that if it's buildable and mostly works, that we turn it on even with known issues.
(In reply to Michael Wu [:mwu] from comment #15)
> No, but we previously agreed not to turn it on until it would actually be
> used. No need to increase our codesize until we need it.

I just check the binary size when build with/without skia configuration.
It only increases about 460 KB and overall memory increase about 4 MB.

In my opinion, it is still acceptable.
And it could reduce the works for people who want to try skiaGL and they could easily change the preference to enable it.

For skiaGL on B2g, I could run the guimark2 and cut the rope with better performance.
For other benchmarks, we may have the CPU bottleneck on "flush" or memory not enough issues.

Later I will check any other testing items failed on B2G with SkiaGL enabled.

[libxul file size]
peter@pchang-desktop:~/b2g_debug_leo/objdir-b2g_central_in/dist$ ls -lk b2g/libxul.so
-rwxrwxr-x 1 peter peter 21789 Jul 26 10:42 b2g/libxul.so

peter@pchang-desktop:~/b2g_debug_leo/objdir-b2g_central_in/dist$ ls -lk b2g_noskia/libxul.so
-rwxrwxr-x 1 peter peter 21329 Jul 26 10:31 b2g_noskia/libxul.so


[b2g-info noskia]
            Total 384.9 MB
     Used - cache 107.0 MB
  B2G procs (PSS)  99.6 MB
    Non-B2G procs   7.4 MB
     Free + cache 277.9 MB
             Free 181.5 MB
            Cache  96.4 MB

[b2g-info skia]
            Total 384.9 MB
     Used - cache 109.4 MB
  B2G procs (PSS) 102.3 MB
    Non-B2G procs   7.1 MB
     Free + cache 275.5 MB
             Free 177.7 MB
            Cache  97.9 MB
Ok, so 4MB memory is _a lot_. Thats 4% of the total available memory or so. That is definitely not ok. Why does skia take up so much memory disabled?
(In reply to Andreas Gal :gal from comment #19)
> Ok, so 4MB memory is _a lot_. Thats 4% of the total available memory or so.
> That is definitely not ok. Why does skia take up so much memory disabled?

I just re-check the memory usage of b2g with/without skia configuration.
And the following are the average data for rebooting to home page 6 times.

I could only see the big difference on cache part, for B2G or non B2G process are almost the same.

Any idea to profile more detail?

[With skia configuration]
            Total	384.9
     Used - cache	104.28
  B2G procs (PSS)	99.14
    Non-B2G procs	5.14
     Free+ cache	280.6
             Free	220.34
            Cache	60.24

[Without skia configuration]
            Total	384.9
     Used - cache	104.34
  B2G procs (PSS)	99.16
    Non-B2G procs	5.18
     Free+ cache	280.54
             Free	215.36
            Cache	65.2

[Raw data]
http://www.pastebin.mozilla.org/2714268
More cached is probably fine. I think that just means we transitioned more data through the cache. The  memory is still available if needed. pchang, when will we officially enable skia in b2g?
It's good on Android, this is the first step in enabling it in b2g.  We expect the stability to get better once bug 858914 lands, but we need to code in the build so that we can pref it on and off easier. Peter is landing this in part so that Benoit can work on this at the same time.
We do expect caching to be a big part of the SkiaGL though, so the extra memory usage is something we need to sort out.
This patch flips the preferences enabling Skia/GL on B2G... we want to first fix regressions before we land that. (Will file as blockers of this bug).
Depends on: 899341
Assignee: mwu → bjacob
Try push of graphics branch (Friday) + enabling skia/GL prefs on B2G:
https://tbpl.mozilla.org/?tree=Try&rev=af9e1082e710
Attachment #627421 - Attachment is obsolete: true
Argh, no tests... trying with "emulator" even though trychooser says "no tests yet".
https://tbpl.mozilla.org/?tree=Try&rev=6169407755d2
Depends on: 899798
Depends on: 900201
Depends on: 897727
Depends on: 905141
Depends on: 905214
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 905227
You need to log in before you can comment on or make changes to this bug.