Filip Pizlo | 23 May 03:41 2016
Picon

RFC: stop using std::chrono, go back to using doubles for time

Hi everyone!

I’d like us to stop using std::chrono and go back to using doubles for time.  First I list the things that I think we wanted to get from std::chrono - the reasons why we started switching to it in the first place.  Then I list some disadvantages of std::chrono that we've seen from fixing std::chrono-based code.  Finally I propose some options for how to use doubles for time.

Why we switched to std::chrono

A year ago we started using std::chrono for measuring time.  std::chrono has a rich typesystem for expressing many different kinds of time.  For example, you can distinguish between an absolute point in time and a relative time.  And you can distinguish between different units, like nanoseconds, milliseconds, etc.

Before this, we used doubles for time.  std::chrono’s advantages over doubles are:

Easy to remember what unit is used: We sometimes used doubles for milliseconds and sometimes for seconds.  std::chrono prevents you from getting the two confused.

Easy to remember what kind of clock is used: We sometimes use the monotonic clock and sometimes the wall clock (aka the real time clock).  Bad things would happen if we passed a time measured using the monotonic clock to functions that expected time measured using the wall clock, and vice-versa.  I know that I’ve made this mistake in the past, and it can be painful to debug.

In short, std::chrono uses compile-time type checking to catch some bugs.

Disadvantages of using std::chrono

We’ve seen some problems with std::chrono, and I think that the problems outweigh the advantages.  std::chrono suffers from a heavily templatized API that results in template creep in our own internal APIs.  std::chrono’s default of integers without overflow protection means that math involving std::chrono is inherently more dangerous than math involving double.  This is particularly bad when we use time to speak about timeouts.

Too many templates: std::chrono uses templates heavily.  It’s overkill for measuring time.  This leads to verbosity and template creep throughout common algorithms that take time as an argument.  For example if we use doubles, a method for sleeping for a second might look like sleepForSeconds(double).  This works even if someone wants to sleep for a nanoseconds, since 0.000001 is easy to represent using a double.  Also, multiplying or dividing a double by a small constant factor (1,000,000,000 is small by double standards) is virtually guaranteed to avoid any loss of precision.  But as soon as such a utility gets std::chronified, it becomes a template.  This is because you cannot have sleepFor(std::chrono::seconds), since that wouldn’t allow you to represent fractions of seconds.  This brings me to my next point.

Overflow danger: std::chrono is based on integers and its math methods do not support overflow protection.  This has led to serious bugs like https://bugs.webkit.org/show_bug.cgi?id=157924.  This cancels out the “remember what unit is used” benefit cited above.  It’s true that I know what type of time I have, but as soon as I duration_cast it to another unit, I may overflow.  The type system does not help!  This is insane: std::chrono requires you to do more work when writing multi-unit code, so that you satisfy the type checker, but you still have to be just as paranoid around multi-unit scenarios.  Forgetting that you have milliseconds and using it as seconds is trivially fixable.  But if std::chrono flags such an error and you fix it with a duration_cast (as any std::chrono tutorial will tell you to do), you’ve just introduced an unchecked overflow and such unchecked overflows are known to cause bugs that manifest as pages not working correctly.

I think that doubles are better than std::chrono in multi-unit scenarios.  It may be possible to have std::chrono work with doubles, but this probably implies us writing our own clocks.  std::chrono’s default clocks use integers, not doubles.  It also may be possible to teach std::chrono to do overflow protection, but that would make me so sad since using double means not having to worry about overflow at all.

The overflow issue is interesting because of its implications for how we do timeouts.  The way to have a method with an optional timeout is to do one of these things:

- Use 0 to mean no timeout.
- Have one function for timeout and one for no timeout.
- Have some form of +Inf or INT_MAX to mean no timeout.  This makes so much mathematical sense.

WebKit takes the +Inf/INT_MAX approach.  I like this approach the best because it makes the most mathematical sense: not giving a timeout is exactly like asking for a timeout at time-like infinity.  When used with doubles, this Just Works.  +Inf is greater than any value and it gets preserved properly in math (+Inf * real = +Inf, so it survives gracefully in unit conversions; +Inf + real = +Inf, so it also survives absolute-to-relative conversions).

But this doesn’t work with std::chrono.  The closest thing to +Inf is duration::max(), i.e. some kind of UINT_MAX, but this is guaranteed to overflow anytime it’s converted to a more precise unit of time (nanoseconds::max() converted to milliseconds is something bogus).  It appears that std::chrono doesn’t have a good story for infinite timeout, which means that anyone writing a function that can optionally have a timeout is going to have a bad time.  We have plenty of such functions in WebKit.  For example, I’m not sure how to come up with a feel-good solution to https://bugs.webkit.org/show_bug.cgi?id=157937 so long as we use std::chrono.

Going back to doubles

Considering these facts, I propose that we switch back to using doubles for time.  We can either simply revert to the way we used doubles before, or we can come up with some more sophisticated approach that blends the best of both worlds.  I prefer plain doubles because I love simplicity.

Simply revert to our old ways: I like this approach the best because it involves only very simple changes.  Prior to std::chrono, we used a double to measure time in seconds.  It was understood that seconds was the default unit.  We would use both monotonic and wall clocks, and we used double for both of them.

Come up with a new type system: Having learned from std::chrono and doubles, it seems that the best typesystem for time would comprise three classes: Seconds, WallTime, and MonotonicTime.  Seconds would be a class that holds a double and supports +/+=/-/-=/</<=/>/>=/==/!= operations, as well as conversions to a raw double for when you really need it.  WallTime and MonotonicTime would be wrappers for Seconds with a more limited set of available operations.  You can convert WallTime or MonotonicTime to Seconds and vice-versa, but some operators are overloaded to make casts unnecessary in most cases (WallTime + Seconds = WallTime, WallTime - WallTime = Seconds, etc).  This would save us from forgetting the unit or the clock.  The name of the Seconds class is a dead give-away, and WallTime and MonotonicTime will not yield you a value that is unit-sensitive unless you say something like WallTime::toSeconds().  There will be no easy way to convert WallTime to MonotonicTime and vice-versa, since we want to discourage such conversions.

Personally I feel very comfortable with doubles for time.  I like to put the word “Seconds” into variable names and function names (waitForSeconds(double) is a dead give-away).  On the other hand, I sort of like the idea of a type system to protect clock mix-ups.  I think that’s the biggest benefit we got from std::chrono.

If it was entirely up to me, I’d go for doubles.  I think that there needs to be a high burden of proof for using types to catch semantic bugs.  A type system *will* slow you down when writing code, so the EV (expected value) of the time savings from bugs caught early needs to be greater than the EV of the time lost due to spoonfeeding the compiler or having to remember how to use those classes.  Although I know that using doubles sometimes meant we had bugs, I don’t think they were frequent or severe enough for the odds to be good for the Seconds/WallTime/MonotonicTime solution.

Thoughts?

-Filip

_______________________________________________
webkit-dev mailing list
webkit-dev <at> lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Daniel Olegovich Lazarenko | 19 May 17:41 2016
Picon
Gravatar

Networking proxy on iOS

Hello,

I'd like to ask your for advice about implementation of a custom networking layer with WKWebView on iOS.

Our current solution is based on NSURLProtocol, and the issues we had with it in 2014 are unresolved:

It was kind of a shoehorn hack, and so it was rejected by Benjamin Poulain and Alexey Proskuryakov among other reviewers.

Now I'm again looking for a better solution.
I'd really like to discuss it with somebody responsible, reach common understanding and agreement, and attack this problem.

There's currently 2 solutions I'm weighting:
  1. Pass and use NetworkProcessCreationParameters.httpProxy to NSURLSessionConfiguration (in NetworkSession and maybe other places).
  2. Add a new mode to the NetworkProcess, which would do all networking in UIProcess (instead of spawning a new process). A mode would be optional and controlled with some configuration setting (or NSUserDefaults).
The httpProxy solution is easy to implement and would look clean design-wise. It would let us spawn an HTTP proxy on localhost and filter the traffic there. There might be some complications, because it's not fully transparent to the client side. For example HTTPS will have issues. All in all this could be a fine short-term solution.

The UIProcess solution is harder to implement, and it will affect more code. It is somewhat controversial. One of the reasons of splitting out a NetworkProcess was to have it respawn after crashes. Nevertheless we can take this risk, because in practice we know that most of the crashes happen in the WebProcess parts. I don't see any other significant downsides of having the UIProcess handling networking. To me this seems like a better choice than httpProxy, because this way we avoid unnecessary data passing back and forth. We get more control, because it's transparent. In addition it can simplify the NetworkProcess debugging.

--
With best regards,
Daniel Lazarenko

_______________________________________________
webkit-dev mailing list
webkit-dev <at> lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Keith Miller | 13 May 01:02 2016
Picon

Importing Test262 into WebKit

Hi everyone,

For those of you that have not already heard of Test262, it is a continually updated conformance test suite
for upcoming ECMAScript standards (https://github.com/tc39/test262). I think it’s in our best
interest to include the test suite in our normal testing infrastructure for JavaScriptCore. The current
plan is to only run the Test262 tests when running run-javascript-core tests. The test suite is fairly
large (~15000 tests) and the last time I ran it, it took about ~15-20 minutes to run. Are there any questions
or concerns with this proposal?

Cheers,
Keith
_______________________________________________
webkit-dev mailing list
webkit-dev <at> lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Brady Eidson | 12 May 01:10 2016
Picon

https://bugs.webkit.org/show_bug.cgi?id=157591)

It is a common pattern throughout the project to temporarily protect a RefCounted object with a Ref/RefPtr.

e.g.,
RefPtr<Node> protectedNode(node);

The naming for these protector variables is all over the map.
e.g.,

RefPtr<Element> protect(element);
RefPtr<Element> protector(this);
RefPtr<Node> self(node);
RefPtr<Widget> widgetRef(widget);

I’ve seen this come up in patch review a lot, most recently from Darin - (https://bugs.webkit.org/show_bug.cgi?id=157448#c16)

In reply (https://bugs.webkit.org/show_bug.cgi?id=157448#c17) I suggested that we should formalize
a style guideline for this so it’s no longer a gray area.

I filed https://bugs.webkit.org/show_bug.cgi?id=157591 with a description of what I think the rule
should be, including examples of both good and bad names.

I’ve already attached a patch to implement the check-webkit-style enforcement of the rule as well as
update https://webkit.org/code-style-guidelines/ describing it.

If there are no objections here in the next ~day, assuming I get a review on the patch, I’ll be landing the
new rule.

Thanks,
~Brady
_______________________________________________
webkit-dev mailing list
webkit-dev <at> lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
jongdeok.kim | 11 May 04:31 2016

Event listeners of the same type in DOM Level 0 event handling,

Hi Everyone,

 

A web page that I'm testing with webkit, uses two event listeners about load event.

It registered those by assigning to window.onload and setting as an 'onload' attribute of body element.

 

On webkit, last registered one replaces previous one, in this case 'onload' attribute wins.

I checked other browsers, but only chrome permits two listeners on same event type.

 

I googled and found out that it's DOM Level 0 event handling.

And I'm wondering this is a webkit bug when using a mix of traditional model and inline model (as referred to below), or implementor dependent.

 

https://en.wikipedia.org/wiki/DOM_events#DOM_Level_0 

 

<html> 

<head> 

<title>onloadload</title> 

<script type="text/javascript"> 

function load() {

    alert("body.onload");

}

window.onload = function () { // traditional model

    alert("window.onload");

};

</script> 

</head> 

<body onload="load()"> <!-- inline model -->

<p>onload.</p> 

</body> 

</html> 

 

Thanks in advance.

jongdeok.

 

_______________________________________________
webkit-dev mailing list
webkit-dev <at> lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Brent Fulgham | 3 May 18:46 2016
Picon

Windows Bot Updates

Hi Everyone,

I have just reviewed the full set of Windows EWS and build bots, and updated them as follows:

1. Perl 5.18 or newer, which matches the Perl shipped with Mac OS.
2. Visual Studio 2015, Update 2. This fixes a number of C++ compatibility issues.

At this point, it should not be necessary to have Windows-specific C++ hacks in the code.

Most of the places where I knew we had worked around VC++ compiler bugs were commented. We have now removed
all such commented hacks.

Please let me know if you are aware of any other cases where we were forced to use less efficient or more
complicated code to work around an issue with Visual C++. We should be able to remove those hacks now.

Thanks,

-Brent
Osztrogonác Csaba | 3 May 16:38 2016
Picon

EWS: feeder and style queues are offline

Hi,

it seems feeder and style queues are offline
now, which means EWS and CQ bots aren't fed.

Could somebody possibly restart these bots?

br,
Ossy
Gyuyoung Kim | 3 May 15:24 2016
Gravatar

On/Off selection gap painting during the text selection

Hello all,

I upload a patch to add a preference API in order to enable/disable the selection gap painting feature.

Add WKPreference for SelectionPaintingWithoutSelectionGaps


You're able to see what is the selection gap painting in below screenshots.


1. Enable selection gap painting on WebKitGTK+ (Red circles point out the selection gaps)




2. Disable selection gap painting on WebKitGTK+





Almost browsers don't support the selection gap painting when doing text selection. So I wonder why WebKit has been
doing support this feature so far. Anyway I think it would be good if we can give each port a functionality to enable/disable
this feature. Because I believe there is definitely ports which want to disable it.

In my opinion this new preference API looks good for WK2. But If there is any concern or comment about new preference API
to WK2, please let me know.

gyuyoung.
_______________________________________________
webkit-dev mailing list
webkit-dev <at> lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Rik Cabanier | 2 May 22:45 2016
Picon
Gravatar

canvas and sRGB

All,

with the release of DCI-P3 screen, WebKit began supporting the display of high gamut images.
Specifically, if you have an image with a DCI-P3 profile, its pixels render untouched on the new displays.

However, if you try do do any sort of canvas manipulation, you will see that the colors are being compressed to sRGB and you will lose the depth of the color.

Was it an oversight to always create the canvas imagebuffer in sRGB? [1]
If this is as-designed, how can we work around this limitation?

PS
I asked the same question on WhatWG. [2]



_______________________________________________
webkit-dev mailing list
webkit-dev <at> lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Osztrogonác Csaba | 27 Apr 14:55 2016
Picon

Are there any plans to upgrade bugzilla server on bugs.webkit.org?

Hi,

The actual topic is upgrading, let's discuss about bugzilla too.

bugs.webkit.org still uses the ancient and unsecure 4.2.11 bugzilla.
The last release from 4.2 series is 4.2.16 (2015 Dec 22) which means
the EOL of 4.2. The latest releases are 4.4.11 and 5.0.2.

Once we could upgrade to 4.4+, a very old annoying bug would be fixed:
https://bugs.webkit.org/show_bug.cgi?id=124047 . (If you review a
patch, you aren't cc-ed to the bug to be able to follow the future
of the bug. You won't be notified if the author fixes what you asked
or if the patch breaks the build or makes tests fail.)

[1] https://www.bugzilla.org/news/
[2] https://bugs.webkit.org/show_bug.cgi?id=55882

br,
Ossy
Osztrogonác Csaba | 27 Apr 14:33 2016
Picon

Re: Are there any plans to upgrade SVN server on svn.webkit.org?

+1 for upgrading the SVN server.

1.6.11 is really outdated and I think it is unsecure too.
1.6.11 was released 6 years ago and the last release
from 1.6 series is 1.6.23 which was released 3 years ago.

https://subversion.apache.org/docs/release-notes/release-history.html
https://en.wikipedia.org/wiki/Apache_Subversion

br,
Ossy

Konstantin Tokarev írta:
> Hello,
> 
> According to [1], currently svn.webkit.org runs severely outdated Subversion 1.6.11. Since then,
there were 3 significant releases of Subversion, which brought lots of performance improvements, most
importantly brand new HTTP protocol [3] and storage format improvements [4]. Also, since 1.7 Apache 2.4
with MPM event is supported, which may increase server throughput.
> 
> 
> BTW, to improve performance of up to date Subversion client (>= 1.8), it is needed to adjust Apache
configuration[5]: increase MaxKeepAliveRequests from default 100 to at least 1000. This needs to be
done regardless of Subversion server upgrade.
> 
> 
> [1] http://svn.webkit.org/repository/webkit/
> [2] https://subversion.apache.org/docs/release-notes/1.7.html
>     https://subversion.apache.org/docs/release-notes/1.8.html
>     https://subversion.apache.org/docs/release-notes/1.9.html
> [3] https://subversion.apache.org/docs/release-notes/1.7.html#httpv2
> [4] https://subversion.apache.org/docs/release-notes/1.8.html#fsfs-enhancements
>     https://subversion.apache.org/docs/release-notes/1.9.html#fsfs-improvements
> [5] https://subversion.apache.org/docs/release-notes/1.8.html#neon-deleted
>     http://svn.haxx.se/dev/archive-2011-01/0320.shtml

Gmane