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

Dart2JS should use Promise.then for scheduleImmediate #20055

Open
floitschG opened this issue Jul 15, 2014 · 9 comments
Open

Dart2JS should use Promise.then for scheduleImmediate #20055

floitschG opened this issue Jul 15, 2014 · 9 comments
Labels
area-web Issues related to Dart Web. customer-google3 dart2js-optimization P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug web-dart2js

Comments

@floitschG
Copy link
Contributor

Dart2js should use DOM Promises when available to scheduleImmediate.

Currently FF is broken and doesn't allow us to switch.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1038631

@floitschG floitschG self-assigned this Jul 15, 2014
@kevmoo kevmoo added status-blocked Blocked from making progress by another (referenced) issue and removed waiting labels Feb 8, 2016
@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug and removed Priority-Medium labels Mar 1, 2016
@floitschG floitschG removed their assignment Aug 11, 2017
@floitschG floitschG removed the status-blocked Blocked from making progress by another (referenced) issue label Aug 11, 2017
@matanlurey
Copy link
Contributor

Dart2JS* still uses a bunch of hacks to get into the native micro-task queue:

@patch
class _AsyncRun {
  @patch
  static void _scheduleImmediate(void callback()) {
    _scheduleImmediateClosure(callback);
  }

  // Lazily initialized.
  static final Function _scheduleImmediateClosure =
      _initializeScheduleImmediate();

  static Function _initializeScheduleImmediate() {
    requiresPreamble();
    if (JS('', 'self.scheduleImmediate') != null) {
      return _scheduleImmediateJsOverride;
    }
    if (JS('', 'self.MutationObserver') != null &&
        JS('', 'self.document') != null) {
      // Use mutationObservers.
      var div = JS('', 'self.document.createElement("div")');
      var span = JS('', 'self.document.createElement("span")');
      var storedCallback;


      internalCallback(_) {
        var f = storedCallback;
        storedCallback = null;
        f();
      }


      var observer = JS('', 'new self.MutationObserver(#)',
          convertDartClosureToJS(internalCallback, 1));
      JS('', '#.observe(#, { childList: true })', observer, div);


      return (void callback()) {
        assert(storedCallback == null);
        storedCallback = callback;
        // Because of a broken shadow-dom polyfill we have to change the
        // children instead a cheap property.
        // See https://github.com/Polymer/ShadowDOM/issues/468
        JS('', '#.firstChild ? #.removeChild(#): #.appendChild(#)', div, div,
            span, div, span);
      };
    } else if (JS('', 'self.setImmediate') != null) {
      return _scheduleImmediateWithSetImmediate;
    }
    // TODO(20055): We should use DOM promises when available.
    return _scheduleImmediateWithTimer;
  }


  static void _scheduleImmediateJsOverride(void callback()) {
    internalCallback() {
      callback();
    }


    JS('void', 'self.scheduleImmediate(#)',
        convertDartClosureToJS(internalCallback, 0));
  }


  static void _scheduleImmediateWithSetImmediate(void callback()) {
    internalCallback() {
      callback();
    }


    JS('void', 'self.setImmediate(#)',
        convertDartClosureToJS(internalCallback, 0));
  }


  static void _scheduleImmediateWithTimer(void callback()) {
    Timer._createTimer(Duration.zero, callback);
  }
}

As of modern browsers, this can be reduced to a single function:

@patch
class _AsyncRun {
  @patch
  static void _scheduleImmediate(void Function() callback) {
    JS('void', 'Promise.prototype.then(#)', convertDartClosureToJS(callback, 0));
  }
}

*(and DDC, I'm not sure? @jmesserly)

@matanlurey matanlurey changed the title dart2js should use DOM Promises when available to scheduleImmediate. Dart2JS should use Promise.then for scheduleImmediate Nov 1, 2018
@matanlurey
Copy link
Contributor

/cc @rakudrama @vsmenon @sigmundch

@jmesserly
Copy link

dart2js still supports IE11, I believe.

for DDC, we can simplify it now.

@matanlurey
Copy link
Contributor

I mean, if under some flag Dart2JS wants to emit the old code, they are welcome. Our biggest and most demanding clients don't need the overhead of running IE11 compatible code. The other option is we could write our own Dart2JS "rewriter" layer internally, but I don't think that's the best option here.

@sigmundch
Copy link
Member

we continue to support IE11 and we are starting a plan to be able to generate better code when IE11 is not needed. However, our plan requires a lot preparation on the compiler and testing infrastructure. Happy to share more details if you are interested, but I don't see us switching to Promise.then for a few months until we are ready.

@rakudrama
Copy link
Member

rakudrama commented Nov 2, 2018 via email

@jmesserly
Copy link

@Markzipan just discovered, this issue causes inconsistent timing with dartdevc on node.js. Future and scheduleMicrotask do not seem to have a consistent ordering, because MutationObservers aren't available (so it falls back to timers). So that might be another reason for us to fix this sooner in DDC.

@jmesserly
Copy link

testing a CL now for DDC (https://dart-review.googlesource.com/c/sdk/+/91765)

@jmesserly
Copy link

jmesserly commented Feb 6, 2019

about to send out the CL. One interesting thing I found: this does cause unhandled exceptions to be treated as unhandled Promise rejects.

In browsers, this means the error goes to the 'unhandledrejection' event rather than the 'error' event, so window.onerror won't catch it. But presumably, JS libraries need to deal with that since Promises are a thing now.

node.js is going to cause unhandled promise rejections to exit the process: https://medium.com/@dtinth/making-unhandled-promise-rejections-crash-the-node-js-process-ffc27cfcc9dd ... for now, it prints a warning. Dart VM behavior is to exit, so it'll be consistent.

edit: also I need to land https://dart-review.googlesource.com/c/sdk/+/92247 before my CL above.

dart-bot pushed a commit that referenced this issue Feb 21, 2019
All of DDC's supported platforms have Promises, so we can use them
instead of MutationObservers (web) and timers (node.js).

See issue #20055 (same issue, but for dart2js).

Change-Id: Id635a4a9fa104a2ab19dd20824d209f682f831f9
Reviewed-on: https://dart-review.googlesource.com/c/91765
Reviewed-by: Mark Zhou <markzipan@google.com>
Commit-Queue: Jenny Messerly <jmesserly@google.com>
@vsmenon vsmenon added the area-web Issues related to Dart Web. label Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Issues related to Dart Web. customer-google3 dart2js-optimization P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug web-dart2js
Projects
None yet
Development

No branches or pull requests

7 participants