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

Firefox timeline integration #4957 #5636

Merged
merged 1 commit into from Apr 13, 2015
Merged

Firefox timeline integration #4957 #5636

merged 1 commit into from Apr 13, 2015

Conversation

@JIoJIaJIu
Copy link
Contributor

JIoJIaJIu commented Apr 10, 2015

Available markers only:

Reflow
DOMEvent
Also need to implement:

Style marker
Paint marker
Javascript marker
frames reply, depends on getting javascript stack
I decided to make pull request before implemented another markers for getting feedback.
mb it would be better to create separated tasks.

Notices:
Marker doesn't fill stack and stackEnd
MemoryActor sends fake data because there is no memory profiler per tab
FramerateActor sends empty Vec, need implement http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#5240

@highfive
Copy link

highfive commented Apr 10, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Apr 10, 2015

Critic review: https://critic.hoppipolla.co.uk/r/4632

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@pcwalton
Copy link
Contributor

pcwalton commented Apr 10, 2015

This is awesome!

@jdm
Copy link
Member

jdm commented Apr 10, 2015

This is close! Most of the comments I left on Critic are just improvements for style and clarity!

@JIoJIaJIu
Copy link
Contributor Author

JIoJIaJIu commented Apr 12, 2015

@pcwalton thanks)

I found two bugs in current implementation:

  1. Firefox crashed after some time (39.0a2)
DBG-SERVER: Got: {
  "type": "markers",
  "markers": [],
  "from": "timeline2",
  "endTime": 113624885915641
}
2015-04-12 19:19:43: stackwalker.cc:125: INFO: Couldn't load symbols for: /home/jiojiajiu/Downloads/firefox/libxul.so|05928FF38ED146CBCE69F9C13E05D4660
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7ffe10fb5700
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7ffe10fb4f20
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7ffe10fb4f00
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7f7a499770f0
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7f7a61e64c00
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7f7a49977108
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7ffe10fb4ff0
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7f7a6209a280
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x37003600000000
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7f7a49977158
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7ffe10fb5aa0
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0xfffc7f7a6205e240
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7ffe10fb50c0
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7ffe10fb5108
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x207f7a766e24a9
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7f7a00000000
2015-04-12 19:19:43: stackwalker.cc:125: INFO: Couldn't load symbols for: /home/jiojiajiu/Downloads/firefox/libnspr4.so|7A2E702309862298A4EA7091B41577990
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7f7a49977158
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7ffe10fb5aa0
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0xfffc7f7a49342240
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x7ffe10fb4f40
2015-04-12 19:19:43: basic_code_modules.cc:88: INFO: No module at 0x0
...
  1. Servo panicked after closing crashed firefox
<- {"type":"markers","markers":[],"from":"timeline2","endTime":113640119597118}
error: EOF
<- {"type":"markers","markers":[],"from":"timeline2","endTime":113640320068868}
thread 'PullTimelineMarkers' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Os(32) }', /home/larsberg/rust/src/libcore/result.rs:744
@jdm
Copy link
Member

jdm commented Apr 12, 2015

The Servo panic comes from write_json_packet not dealing with connection errors. The Firefox hang is really interesting! It looks like we should be returning values in milliseconds, and we're currently providing nanoseconds (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/timeline/marker-details.js#119). Maybe that will help?

@jdm
Copy link
Member

jdm commented Apr 12, 2015

Actually, that's not entirely true - it looks like we want to calculate a high resolution timestamp like https://developer.mozilla.org/en-US/docs/Web/API/Performance/now with fractional milliseconds.

@JIoJIaJIu
Copy link
Contributor Author

JIoJIaJIu commented Apr 13, 2015

@jdm, yes, it looks like you described. Could you create separated task on it, please?
Cause I have a questions: I need use window.performance in devtools crait, should I create one more channel or extend script_channel for getting Performance.now()?

Also about https://critic.hoppipolla.co.uk/showcomment?chain=11699 - I use &mut self in Emitter methods

@jdm
Copy link
Member

jdm commented Apr 13, 2015

Yes, we can use separate issue for the timestamps.

@jdm
Copy link
Member

jdm commented Apr 13, 2015

Go ahead and squash these together!

@JIoJIaJIu JIoJIaJIu force-pushed the JIoJIaJIu:timeline branch from 60fc399 to 97714ec Apr 13, 2015
@JIoJIaJIu
Copy link
Contributor Author

JIoJIaJIu commented Apr 13, 2015

@jdm done

@jdm
Copy link
Member

jdm commented Apr 13, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2015

📌 Commit 97714ec has been approved by jdm

bors-servo pushed a commit that referenced this pull request Apr 13, 2015
Available markers only:

Reflow
DOMEvent
Also need to implement:

Style marker
Paint marker
Javascript marker
frames reply, depends on getting javascript stack
I decided to make pull request before implemented another markers for getting feedback.
mb it would be better to create separated tasks.

Notices:
Marker doesn't fill stack and stackEnd
MemoryActor sends fake data because there is no memory profiler per tab
FramerateActor sends empty Vec, need implement http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#5240
@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2015

Testing commit 97714ec with merge 74c847a...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit 97714ec into servo:master Apr 13, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 14, 2015

@JIoJIaJIu: look like this cause a "warning: method is never used: on_refresh_driver_tick"; what should we do with it?

@JIoJIaJIu
Copy link
Contributor Author

JIoJIaJIu commented Apr 14, 2015

@Ms2ger Here it should be called once as a callback of RequestAnimationFrame and then it works recursively.
There is in mozilla firefox #1 and #2
If you will create subtask on me for implementing this it would be nice, cause I decided to make pull request and later fix parts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.