Skip to content

/ servo

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

Jump to bottom

Make Fragment::debug_id only claim space in debug mode #87

Closed
burg opened this issue Sep 22, 2012 · 7 comments
Closed

Make Fragment::debug_id only claim space in debug mode #87

burg opened this issue Sep 22, 2012 · 7 comments
Labels
A-layout/uncategorized C-assigned E-easy I-perf-bloat

Comments

@burg
Copy link

@burg burg commented Sep 22, 2012

For debugging purposes, all flows/boxes are stamped with an int, to make debugging output readable. Somehow, we should only include this field for debug builds. (well, once there is more than one build mode...)
@jjjjw
Copy link
Contributor

@jjjjw jjjjw commented May 26, 2013

Blocked by: #420
@frewsxcv frewsxcv removed the C-upstream label Aug 26, 2015
@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Aug 26, 2015

Update: #420 has been closed
@mbrubeck mbrubeck changed the title Make flow/box numberings only claim space in debug mode Make Fragment::debug_id only claim space in debug mode May 24, 2016
@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented May 24, 2016

This number is currently stored in Fragment::debug_id. This could be fixed by changing this field's type from u16 to a new custom type struct DebugId. Then use conditional compilation to make this struct contain a single u16 field when cfg(debug_assertions) is enabled, or no fields otherwise (so it will take up zero space).

The new type will need to implement the Display trait. In non-debug builds, it could display as an empty string, or maybe as its own address.
@mitchhentges
Copy link
Contributor

@mitchhentges mitchhentges commented May 26, 2016

I bet I can take care of this
@mitchhentges
Copy link
Contributor

@mitchhentges mitchhentges commented May 26, 2016 •
edited

I'm not sure what we want to return from debug_id() if it doesn't exist, so I'll opt for 0. This will affect fmt::Debug for Fragment and Encodable for Fragment

At the same time, I feel like Option(u16) might be better. Time to bug people on IRC
@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented May 26, 2016

You could change debug_id() to return a DebugId instead of u16.
@mitchhentges
Copy link
Contributor

@mitchhentges mitchhentges commented May 26, 2016

Hmm, that will just defer the I'd resolution, but I think I've got some ideas. Memory address sounds good, too
@mitchhentges mitchhentges mentioned this issue May 26, 2016
Merged
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jun 1, 2016
@bors-servo
Auto merge of #11442 - mitchhentges:87-debug-id, r=KiChjang
170a21e 
Fragment debug_id u16 only exists in debug, prod will format mem address

<!-- Please describe your changes on the following line: -->
Each fragment has a `u16` `debug_id` in debug mode, but no `debug_id` in production to save memory. To format a debug id in production, the address of the empty `debug_id` is displayed.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #87 (github issue number if applicable).

<!-- Either: -->
- [X] These changes do not require tests because it looks like it's not possible to mock out `cfg` options in `#[test]`s

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11442)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 4, 2016
@bors-servo
Auto merge of #11442 - mitchhentges:87-debug-id, r=KiChjang
a149d75 
Fragment debug_id u16 only exists in debug, prod will format mem address

<!-- Please describe your changes on the following line: -->
Each fragment has a `u16` `debug_id` in debug mode, but no `debug_id` in production to save memory. To format a debug id in production, the address of the empty `debug_id` is displayed.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #87 (github issue number if applicable).

<!-- Either: -->
- [X] These changes do not require tests because it looks like it's not possible to mock out `cfg` options in `#[test]`s

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11442)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 4, 2016
@bors-servo
Auto merge of #11442 - mitchhentges:87-debug-id, r=KiChjang
Loading status checks…
5002dff 
Fragment debug_id u16 only exists in debug, prod will format mem address

<!-- Please describe your changes on the following line: -->
Each fragment has a `u16` `debug_id` in debug mode, but no `debug_id` in production to save memory. To format a debug id in production, the address of the empty `debug_id` is displayed.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #87 (github issue number if applicable).

<!-- Either: -->
- [X] These changes do not require tests because it looks like it's not possible to mock out `cfg` options in `#[test]`s

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11442)
<!-- Reviewable:end -->
@jdm jdm mentioned this issue Nov 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-layout/uncategorized C-assigned E-easy I-perf-bloat
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Fragment debug_id u16 only exists in debug, prod will format mem address
6 participants
@mbrubeck @jdm @burg @frewsxcv @jjjjw @mitchhentges
You can’t perform that action at this time.