servo / gleam

Generated OpenGL bindings and wrapper for Servo.
Other
83 stars 58 forks source link

Add a wrapper in gleam to time each gl call. #187

Closed bholley closed 5 years ago

bholley commented 5 years ago

Differential Revision: https://phabricator.services.mozilla.com/D22201


This change is Reviewable

bholley commented 5 years ago

@gw3583 Can you merge + release?

asajeffrey commented 5 years ago

I share @nox's concern that this is doubling the number of dynamic function calls.

We might be able to avoid that by something like:

   pub struct ProfilingGl<F, G: Gl> {
    gl: G,
    ...

then adding a method to Gl:

  fn profile<F: Fn(&str, Duration)>(&self, f: F) -> Rc<Gl> {
     let profiled: ProfilingGl<F, Self> = ...;
     Rc::new(profiled)
  }
bholley commented 5 years ago

I tried this for a few hours today and wasn't able to make it work. First I got stuck on not being able to make this a default trait impl. I finally gave up and just implemented on each of the concrete types with a macro, but then I got stuck with the inability to downcast Rc to Rc (There is Rc::downcast, but you can't make an Rc from Rc, only from Rc).

So if either of you can come up with a working alternative, please do. Otherwise, please merge the PR (it was already reviewed by @gw3583 ).

asajeffrey commented 5 years ago

IRC chat: https://mozilla.logbot.info/servo/20190307#c16061804 and https://mozilla.logbot.info/servo/20190308#c16061834 ending with "All this is a lot of work to save some dynamic method dispatches which probably aren't the bottleneck anyway."

jdm commented 5 years ago

@bors-servo r=gw3583

bors-servo commented 5 years ago

:pushpin: Commit 6176517 has been approved by gw3583

bors-servo commented 5 years ago

:hourglass: Testing commit 6176517bc377963149befd1f98c4e4648e1d3514 with merge 3cd66fa011b4e4fc9c1fe1249d66806359625838...

bholley commented 5 years ago

@bors-servo r=gw3583

bors-servo commented 5 years ago

:bulb: This pull request was already approved, no need to approve it again.

bors-servo commented 5 years ago

:pushpin: Commit 6176517 has been approved by gw3583

bors-servo commented 5 years ago

:hourglass: Testing commit 6176517bc377963149befd1f98c4e4648e1d3514 with merge 53316f2bfa41ee2a977d3fe50543d5613601fb61...

bors-servo commented 5 years ago

:sunny: Test successful - checks-travis Approved by: gw3583 Pushing 53316f2bfa41ee2a977d3fe50543d5613601fb61 to master...