Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move handling of subnormals into FutureFeatures. #271

Merged
merged 1 commit into from
Oct 26, 2015

Conversation

sunfishcode
Copy link
Member

With #243 (the subnormal_mode proposal) closed, this PR instead addresses #148 by moving subnormal flushing into FutureFeatures.md.

In the public discussion in #148, there is consensus for supporting subnormals by default in the MVP. And, we have abundant of data confirming that this is practical, since asm.js has subnormals enabled and does not even have any way to disable them.

@jfbastien
Copy link
Member

I'd still like to leave this as "under discussion", since making up our mind on what the default is will be pretty easy.

@sunfishcode
Copy link
Member Author

What are we waiting for?

@jfbastien
Copy link
Member

Data from real implementations. It's easy to figure out in the future, and harder / misleading to put in the FAQ as-is and then change later. I'm OK with the FAQ explaining this, but right now it's phrased as pretty definitive (FTZ is a future feature, denormals are in MVP) and I've had a few folks tell me they didn't like that conclusion (and would rather have the opposite). I think #148 has pretty good points but not consensus, so IMO the FAQ should reflect that.

@titzer
Copy link

titzer commented Jul 23, 2015

I think we should move the FTZ feature to FutureFeatures, reasoning thusly:

1.) it's a performance feature
2.) it's easy to add along the lines discussed, provided data appears that justifies it, but can't really be taken away without leaving a wart
3.) MVP should strive for minimal completeness

@titzer
Copy link

titzer commented Jul 23, 2015

Also

4.) Most implementations will probably ignore FTZ at first

@jfbastien
Copy link
Member

To be clear: FTZ isn't necessarily a "feature" that one turns on or off. It can be the default. Denormal support can be the future feature that one turns on or off, than can get ignored, that breaks minimal completeness, and that can't be removed without leaving a wart.

This is why I want to choose what the default is later, with data. It's easy to set this default, and deciding now seems premature.

@titzer
Copy link

titzer commented Jul 23, 2015

There are lot of good reasons to make IEEE 754 (which requires subnormal
support) the default, as been discussed previously. All hardware
implementations that I am aware of that initially cut corners eventually
tend toward IEEE 754 compliance. That makes sense given that it's a widely
accepted standard. Under that assumption, FTZ or other deviations only make
sense if they improve performance above some threshold, and that needs to
be justified with data.

On Thu, Jul 23, 2015 at 7:58 PM, JF Bastien notifications@github.com
wrote:

To be clear: FTZ isn't necessarily a "feature" that one turns on or off.
It can be the default. Denormal support can be the future feature that one
turns on or off, than can get ignored, that breaks minimal completeness,
and that can't be removed without leaving a wart.

This is why I want to choose what the default is later, with data. It's
easy to set this default, and deciding now seems premature.


Reply to this email directly or view it on GitHub
#271 (comment).

@lukewagner
Copy link
Member

+1

@sunfishcode
Copy link
Member Author

I agree with @jfbastien that data is important. We have just performed an extensive multi-year study into the performance of real-world apps running in the real world on real-world browsers with IEEE-conforming subnormal handling enabled. The data shows that:

  • The vast majority of applications see no significant performance impact due to subnormal handling.
  • Widely-used benchmark suites don't register any performance difference whether subnormal handling is enabled or disabled.
  • Participating game developers (reputed to be among the most demanding on this topic) have not reported subnormal handling as a significant concern in practice.
  • A few specific application domains, such as audio processing, regularly see an impact due to subnormal handling.

With respect to the concerns reported, it is possible that subnormal handling is merely being overshadowed by the lack of threading and SIMD in our study. For WebAssembly, since threads and SIMD are already planned to be postponed until after the MVP, we can expect to see the same overshadowing if that is the case.

With respect to those application domains that are regularly affected, it is unfortunate that we can't provide all features desired by all application domains in the MVP, but this is consistent with the nature of the MVP. We're purposefully keeping the scope restricted in many areas, including many that are far more well known than subnormal handling, to keep the MVP focused.

Implementors' opinions are also important. People from several major browser engines have spoken in favor making IEEE-754 behavior the default and addressing FTZ+DAZ in a future feature.

It may also be noted that the proposal to disable subnormal handling on esdiscuss failed to gain traction. This doesn't mean that it was a bad idea, but it does help confirm that this feature isn't actually a major concern in the big picture, so deferring addressing it until after the MVP is reasonable.

The choice for the MVP is clear.

@sunfishcode sunfishcode modified the milestone: MVP Sep 1, 2015
@titzer
Copy link

titzer commented Oct 23, 2015

Are we OK merging this?

@lukewagner
Copy link
Member

lgtm

titzer pushed a commit that referenced this pull request Oct 26, 2015
…ture

Move handling of subnormals into FutureFeatures.
@titzer titzer merged commit 0bbef77 into master Oct 26, 2015
@sunfishcode sunfishcode deleted the address-subnormals-in-the-future branch October 31, 2015 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants