Allow StringView to have the sole reference to a static string

Allow StringView to have the sole reference to a static string

Static strings are thread safe and can safely have their ref counts
floor at 1, so StringView should not crash if it happens to do that.

BUG=674388
Committed: https://crrev.com/8b89825da13786897274a69663ea06e1c41da9e7
Cr-Commit-Position: refs/heads/master@{#440546}

[Charlie Harrison, 2016-12-22 20:19:35]

csharrison@chromium.org changed reviewers:
+ esprehn@chromium.org

[Charlie Harrison, 2016-12-22 20:19:35]

esprehn: Here's the StringView change as it's own patch.

[esprehn, 2016-12-22 21:21:22]

I think we actually do allow it to hit zero, call destroyIfNotStatic() and then
do notbing in there. Where do you see it floor at 1?

Lgtm

[Charlie Harrison, 2016-12-22 21:23:56]

On 2016/12/22 21:21:22, esprehn wrote:
> I think we actually do allow it to hit zero, call destroyIfNotStatic() and
then
> do notbing in there. Where do you see it floor at 1?

Here is the code I was looking at:

  ALWAYS_INLINE void deref() {
    if (hasOneRef()) {
      destroyIfNotStatic();
      return;
    }

    --m_refCount;
  }

Seems like m_refCount will never go below 1, because we early return at
hasOneRef().


> 
> Lgtm

[Charlie Harrison, 2016-12-22 21:24:13]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-12-22 21:24:33]

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

[esprehn, 2016-12-22 21:44:08]

On 2016/12/22 at 21:23:56, csharrison wrote:
> On 2016/12/22 21:21:22, esprehn wrote:
> > I think we actually do allow it to hit zero, call destroyIfNotStatic() and
then
> > do notbing in there. Where do you see it floor at 1?
> 
> Here is the code I was looking at:
> 
>   ALWAYS_INLINE void deref() {
>     if (hasOneRef()) {
>       destroyIfNotStatic();
>       return;
>     }
> 
>     --m_refCount;
>   }
> 
> Seems like m_refCount will never go below 1, because we early return at
hasOneRef().
> 
> 
> > 
> > Lgtm

Interesting, I think that used to be

if (!--m_refCount) but maybe I'm misremembering. I wonder if that would produce
smaller code hmm...

[Charlie Harrison, 2016-12-22 22:29:26]

On 2016/12/22 21:44:08, esprehn wrote:
> On 2016/12/22 at 21:23:56, csharrison wrote:
> > On 2016/12/22 21:21:22, esprehn wrote:
> > > I think we actually do allow it to hit zero, call destroyIfNotStatic() and
> then
> > > do notbing in there. Where do you see it floor at 1?
> > 
> > Here is the code I was looking at:
> > 
> >   ALWAYS_INLINE void deref() {
> >     if (hasOneRef()) {
> >       destroyIfNotStatic();
> >       return;
> >     }
> > 
> >     --m_refCount;
> >   }
> > 
> > Seems like m_refCount will never go below 1, because we early return at
> hasOneRef().
> > 
> > 
> > > 
> > > Lgtm
> 
> Interesting, I think that used to be
> 
> if (!--m_refCount) but maybe I'm misremembering. I wonder if that would
produce
> smaller code hmm...

Yes, this results in a win of 429.808 kb with GN args [1]. This has behavior
with static strings but we should probably ship it?!

[1]:
is_debug = false
is_official_build = true
is_component_build = false
use_goma = true
enable_nacl = false
symbol_level = 0

[Charlie Harrison, 2016-12-22 22:32:17]

On 2016/12/22 22:29:26, Charlie Harrison OOO Dec 15-21 wrote:
> On 2016/12/22 21:44:08, esprehn wrote:
> > On 2016/12/22 at 21:23:56, csharrison wrote:
> > > On 2016/12/22 21:21:22, esprehn wrote:
> > > > I think we actually do allow it to hit zero, call destroyIfNotStatic()
and
> > then
> > > > do notbing in there. Where do you see it floor at 1?
> > > 
> > > Here is the code I was looking at:
> > > 
> > >   ALWAYS_INLINE void deref() {
> > >     if (hasOneRef()) {
> > >       destroyIfNotStatic();
> > >       return;
> > >     }
> > > 
> > >     --m_refCount;
> > >   }
> > > 
> > > Seems like m_refCount will never go below 1, because we early return at
> > hasOneRef().
> > > 
> > > 
> > > > 
> > > > Lgtm
> > 
> > Interesting, I think that used to be
> > 
> > if (!--m_refCount) but maybe I'm misremembering. I wonder if that would
> produce
> > smaller code hmm...
> 
> Yes, this results in a win of 429.808 kb with GN args [1]. This has behavior
> with static strings but we should probably ship it?!
> 
> [1]:
> is_debug = false
> is_official_build = true
> is_component_build = false
> use_goma = true
> enable_nacl = false
> symbol_level = 0

^^ on Linux

[commit-bot: I haz the power, 2016-12-22 23:47:27]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1482441853663670, "parent_rev":
"eb2e7d17df537a4d4c26d67c7f4eed11971d1799", "commit_rev":
"6342d10b42f6591b932c5e5008de94e75bc6b2ba"}

[commit-bot: I haz the power, 2016-12-22 23:47:53]

Description was changed from

==========
Allow StringView to have the sole reference to a static string

Static strings are thread safe and can safely have their ref counts
floor at 1, so StringView should not crash if it happens to do that.

BUG=674388
==========

to

==========
Allow StringView to have the sole reference to a static string

Static strings are thread safe and can safely have their ref counts
floor at 1, so StringView should not crash if it happens to do that.

BUG=674388

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

[commit-bot: I haz the power, 2016-12-22 23:47:54]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-22 23:50:10]

Description was changed from

==========
Allow StringView to have the sole reference to a static string

Static strings are thread safe and can safely have their ref counts
floor at 1, so StringView should not crash if it happens to do that.

BUG=674388

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

to

==========
Allow StringView to have the sole reference to a static string

Static strings are thread safe and can safely have their ref counts
floor at 1, so StringView should not crash if it happens to do that.

BUG=674388

Committed: https://crrev.com/8b89825da13786897274a69663ea06e1c41da9e7
Cr-Commit-Position: refs/heads/master@{#440546}
==========

[commit-bot: I haz the power, 2016-12-22 23:50:11]

Patchset 1 (id:??) landed as
https://crrev.com/8b89825da13786897274a69663ea06e1c41da9e7
Cr-Commit-Position: refs/heads/master@{#440546}