Bug 237341

Created attachment 453535 [details]
PATCH

Comment on attachment 453535 [details]
PATCH

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

> Source/bmalloc/libpas/src/libpas/pas_monotonic_time.c:88
> +    clock_gettime_np(CLOCK_MONOTONIC, &ts);

Is it possible to use CLOCK_MONOTONIC_FAST in this playstation platform? (FreeBSD has this one).
This function needs to be super fast, so full CLOCK_MONOTONIC is not ideal if the platform has an alternative.

(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 453535 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453535&action=review
> 
> > Source/bmalloc/libpas/src/libpas/pas_monotonic_time.c:88
> > +    clock_gettime_np(CLOCK_MONOTONIC, &ts);
> 
> Is it possible to use CLOCK_MONOTONIC_FAST in this playstation platform?
> (FreeBSD has this one).
> This function needs to be super fast, so full CLOCK_MONOTONIC is not ideal
> if the platform has an alternative.

Sure. Sorry, I've forgotten your advice.

Created attachment 453539 [details]
PATCH

Comment on attachment 453539 [details]
PATCH

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

> Source/bmalloc/libpas/src/libpas/pas_monotonic_time.c:89
> +    return ts.tv_sec * 1.0e9 + ts.tv_nsec;

Do you really want to do floating point double precision multiplication here, and then convert back to integer? Maybe uint64_t multiplication would be better?

(In reply to Darin Adler from comment #5)
> Comment on attachment 453539 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453539&action=review
> 
> > Source/bmalloc/libpas/src/libpas/pas_monotonic_time.c:89
> > +    return ts.tv_sec * 1.0e9 + ts.tv_nsec;
> 
> Do you really want to do floating point double precision multiplication
> here, and then convert back to integer? Maybe uint64_t multiplication would
> be better?

Yusuke, what do you think? I think Darin's point is reasonable, but any reason to use float? I've followed Linux implementation.

We need to add special rule to check-webkit-style for libpas C files...
https://ews-build.webkit.org/#/builders/6/builds/70223

Comment on attachment 453539 [details]
PATCH

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

>>> Source/bmalloc/libpas/src/libpas/pas_monotonic_time.c:89
>>> +    return ts.tv_sec * 1.0e9 + ts.tv_nsec;
>> 
>> Do you really want to do floating point double precision multiplication here, and then convert back to integer? Maybe uint64_t multiplication would be better?
> 
> Yusuke, what do you think? I think Darin's point is reasonable, but any reason to use float? I've followed Linux implementation.

I think either is fine.

https://godbolt.org/z/dq34fn6xz

Quick code comparison seems compact and looks faster. I'm building this and comparing with JetStream2 result.

original:
Total Score:  28.991

integer version:
Total Score:  29.347

So the later is a bit faster. I'll go with this for PlayStation port. Thanks!

Created attachment 453568 [details]
PATCH

Thanks, Yusuke and Darin.

Committed r290719 (247966@main): <https://commits.webkit.org/247966@main>

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

<rdar://problem/89665276>

Comment on attachment 453568 [details]
PATCH

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

> Source/bmalloc/libpas/src/libpas/pas_monotonic_time.c:89
> +    return ts.tv_sec * 1000u * 1000u * 1000u + ts.tv_nsec;

This looks wrong given C type promotion rules. The multiplication will not be done as uint64_t, since the things we are multiplying are unsigned (32 bit). So it may be truncated at 32 bits unless tv_sec already a has 64-bit type, then addition also done as 32-bit and only then put into 64-bit. Need to convert one of the integers being multiplied to uint64_t before multiplying. I suggest putting tv_sec into a uint64_t local.