Bug 107312 – Weak GC maps should be easier to use

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Weak GC maps should be easier to use

[Geoffrey Garen]

Created attachment 183530 [details]
Patch

[Geoffrey Garen]

Comment on attachment 183530 [details]
Patch

Sam Weinig reviewed this.

[Geoffrey Garen]

Committed r140194: <http://trac.webkit.org/changeset/140194>

[Jer Noble]

Build fixes:

Committed r140199: <http://trac.webkit.org/changeset/140199>
Committed r140200: <http://trac.webkit.org/changeset/140200>

[Viatcheslav Ostapenko]

Created attachment 183537 [details]
For some reason "using" doesn't  work on linuxes. Replace it with "typedef".

[Viatcheslav Ostapenko]

Reopened because of build break.

[Ryosuke Niwa]

Comment on attachment 183537 [details]
For some reason "using" doesn't  work on linuxes. Replace it with "typedef".

rs=me.

[Ryosuke Niwa]

Committed r140211: <http://trac.webkit.org/changeset/140211>

[Csaba Osztrogonác]

You could have avoided 2.5 hours and 20 commit long build breakage ,
if you waited more than 18 seconds for the EWS bots ...

[Geoffrey Garen]

Reopening to attach new patch.

[Geoffrey Garen]

Created attachment 183692 [details]
Patch

[Geoffrey Garen]

Committed r140284: <http://trac.webkit.org/changeset/140284>

[Darin Adler]

Comment on attachment 183692 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183692&action=review

> Source/JavaScriptCore/runtime/WeakGCMap.h:73
>      AddResult add(const KeyType& key, MappedPassInType value)
>      {
>          gcMapIfNeeded();
> -        AddResult addResult = Base::add(key, value);
> -        if (!addResult.isNewEntry && !addResult.iterator->value) { // Found a zombie value.
> +        AddResult addResult = Base::add(key, nullptr);
> +        if (!addResult.iterator->value) { // New value or found a zombie value.
>              addResult.isNewEntry = true;
>              addResult.iterator->value = value;
>          }

To me this function looks like HashMap::set. Three questions:

1) Why is it right to name the operation add rather than set?
2) Why doesn’t this call Base::set rather than Base::add?
3) Does HashMap::set have the same bug that you fixed here? If not, why not?

[Geoffrey Garen]

> 3) Does HashMap::set have the same bug that you fixed here? If not, why not?

I wouldn't call this a bug in HashMap -- it's just a unique feature of weak values.

The key new feature in WeakGCMap, which isn't appropriate for HashSet, is that values can magically become null, and a null value is treated "as if" there were no such entry in the table.

Perhaps HashMap could be extended to support this feature naturally through an additional value trait. Then we could remove WeakGCMap entirely. I'm not sure if that would be appropriate or not.

> 1) Why is it right to name the operation add rather than set?

If there is a non-null value in the table, this function will not overwrite the value.

> 2) Why doesn’t this call Base::set rather than Base::add?

Same reason.