Bug 230331

Summary: Various tweaks in preparation for Temporal.Instant
Product: WebKit Reporter: Philip Chimento <philip.chimento>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, philip.chimento, ross.kirsling, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 229826    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Philip Chimento 2021-09-15 17:35:48 PDT
Various tweaks in preparation for Temporal.Instant
Comment 1 Philip Chimento 2021-09-15 17:37:33 PDT
Created attachment 438310 [details]
Patch
Comment 2 Yusuke Suzuki 2021-09-15 18:31:56 PDT
Comment on attachment 438310 [details]
Patch

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

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:146
> -            throwRangeError(globalObject, scope, "Could not parse Duration string"_s);
> +            throwRangeError(globalObject, scope, makeString("'"_s, string, "' is not a valid Duration string"_s));

Need to handle the case string is INT32_MAX size already.
We need to truncate the string if it is too long.

> Source/JavaScriptCore/runtime/TemporalObject.cpp:65
> -static JSValue createNowObject(VM& vm, JSObject* object)
> +static JSValue createDurationConstructor(VM& vm, JSObject* object)
>  {
>      TemporalObject* temporalObject = jsCast<TemporalObject*>(object);
>      JSGlobalObject* globalObject = temporalObject->globalObject(vm);
> -    return TemporalNow::create(vm, TemporalNow::createStructure(vm, globalObject));
> +    return TemporalDurationConstructor::create(vm, TemporalDurationConstructor::createStructure(vm, globalObject, globalObject->functionPrototype()), jsCast<TemporalDurationPrototype*>(globalObject->durationStructure()->storedPrototypeObject()));
>  }
>  
> -static JSValue createDurationConstructor(VM& vm, JSObject* object)
> +static JSValue createNowObject(VM& vm, JSObject* object)
>  {
>      TemporalObject* temporalObject = jsCast<TemporalObject*>(object);
>      JSGlobalObject* globalObject = temporalObject->globalObject(vm);
> -    return TemporalDurationConstructor::create(vm, TemporalDurationConstructor::createStructure(vm, globalObject, globalObject->functionPrototype()), jsCast<TemporalDurationPrototype*>(globalObject->durationStructure()->storedPrototypeObject()));
> +    return TemporalNow::create(vm, TemporalNow::createStructure(vm, globalObject));
>  }
>  

I don't think these changes are necessary.

> Source/JavaScriptCore/runtime/TemporalObject.cpp:171
> +String temporalUnitToString(TemporalUnit unit)
> +{
> +    switch (unit) {
> +    case TemporalUnit::Year: return "year"_s;
> +    case TemporalUnit::Month: return "month"_s;
> +    case TemporalUnit::Week: return "week"_s;
> +    case TemporalUnit::Day: return "day"_s;
> +    case TemporalUnit::Hour: return "hour"_s;
> +    case TemporalUnit::Minute: return "minute"_s;
> +    case TemporalUnit::Second: return "second"_s;
> +    case TemporalUnit::Millisecond: return "millisecond"_s;
> +    case TemporalUnit::Microsecond: return "microsecond"_s;
> +    case TemporalUnit::Nanosecond: return "nanosecond"_s;
> +    }
> +    ASSERT_NOT_REACHED();
> +}

Use TemporalDuration.cpp's propertyName.
Comment 3 Yusuke Suzuki 2021-09-15 18:32:41 PDT
Comment on attachment 438310 [details]
Patch

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

>> Source/JavaScriptCore/runtime/TemporalDuration.cpp:146
>> +            throwRangeError(globalObject, scope, makeString("'"_s, string, "' is not a valid Duration string"_s));
> 
> Need to handle the case string is INT32_MAX size already.
> We need to truncate the string if it is too long.

Can you add a test passing super long string? `"T".repeat(int32_max)`
Comment 4 Ross Kirsling 2021-09-15 19:30:10 PDT
Comment on attachment 438310 [details]
Patch

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

> Source/JavaScriptCore/runtime/ISO8601.h:65
> +    double& operator[](TemporalUnit u) { return m_data[static_cast<uint8_t>(u)]; }
> +    const double& operator[](TemporalUnit u) const { return m_data[static_cast<uint8_t>(u)]; }

Hmm, it doesn't seem like you're using this in this patch, though -- I think it should be added just when the need arises.
Comment 5 Philip Chimento 2021-09-16 17:54:13 PDT
Created attachment 438421 [details]
Patch
Comment 6 Philip Chimento 2021-09-16 17:56:23 PDT
Thanks for the reviews. I've removed the functions that are not directly used in this patch. That combined with rebasing this over recent changes has made this patch much smaller :-)
Comment 7 Philip Chimento 2021-09-17 10:07:07 PDT
Any idea why the `"T".repeat(int32_max)` test might be crashing on armv7?
Comment 8 Philip Chimento 2021-09-22 15:36:49 PDT
(In reply to Philip Chimento from comment #7)
> Any idea why the `"T".repeat(int32_max)` test might be crashing on armv7?

I am reasonably sure now this is because the EWS bots just run out of memory when you do this. The tests seem to crash with SIGKILL not SIGSEGV. I've been trying this on a rpi4 and I also get SIGKILL when just running `'T'.repeat(2147483647)` at the JSC shell prompt. So maybe we shouldn't have this test after all?
Comment 9 Radar WebKit Bug Importer 2021-09-22 17:36:21 PDT
<rdar://problem/83425035>
Comment 10 Philip Chimento 2021-09-22 17:46:40 PDT
Created attachment 438998 [details]
Patch
Comment 11 Philip Chimento 2021-09-22 17:49:11 PDT
For now I've removed the test that exhausts the armv7 buildbot's memory. Also new in this revision is a small bugfix to handle { fractionalSecondDigits: NaN } according to the specification. Finally I moved TemporalDuration::unitPropertyName() to TemporalObject.cpp as discussed with Ross, and renamed it temporalUnitPropertyName().

If we want the T.repeat(int32max) test back, maybe there is a way to disable it on the buildbots that can't handle it?
Comment 12 Ross Kirsling 2021-09-22 18:10:19 PDT
Comment on attachment 438998 [details]
Patch

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

> Source/JavaScriptCore/runtime/TemporalObject.cpp:242
> +        if (std::isnan(doubleValue) || doubleValue < 0 || doubleValue > 9) {

Ahh, we could do it this way, but this is actually me being silly -- I should have written `if(!(doubleValue >= 0 && doubleValue <= 9))`, as Yusuke did in TemporalPlainTime.
Comment 13 Yusuke Suzuki 2021-09-22 18:18:50 PDT
Comment on attachment 438998 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:122
> +        copy.append(UChar { 0x2026 }); // U+2026 ellipsis

Use `horizontalEllipsis` in <wtf/unicode/CharacterNames.h>

> Source/JavaScriptCore/runtime/TemporalObject.cpp:254
> +        throwRangeError(globalObject, scope, makeString("fractionalSecondDigits must be 'auto' or 0 through 9, not "_s, stringValue));

Also need to consider about super long string. So ellipsizeAt is necessary.
Comment 14 Philip Chimento 2021-09-23 09:42:08 PDT
Created attachment 439052 [details]
Patch
Comment 15 EWS 2021-09-23 14:50:58 PDT
Committed r283009 (242074@main): <https://commits.webkit.org/242074@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439052 [details].