Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
346033003:
[XHR] Move bools to end of class declaration for better padding (Closed)
Description[XHR] Move bools to end of class declaration for better padding
Move all 7 bool members of class XHRHttpRequest to end of declaration.
This results in better padding and reduces class size by 16 bytes.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176859
Patch Set 1 #
Total comments: 12
Patch Set 2 : Review comments #Patch Set 3 : Move bools to end of class declaration for better padding #
Messages
Total messages: 16 (0 generated)
PTAL
For the record, I don't know that it is worth having flags here (and thus more code complexity and bit operations) to reduce the size of XMLHttpRequest. I don't think we are keeping a lot of these requests around so I am not entirely convinced of the benefits (I'll let Eric or Esprehn review this change). https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.cpp:183: m_xhrFlags = IsAsync | IsUploadEventsAllowed | IsSameOriginRequest; Why isn't this done in the initializer list? https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... File Source/core/xml/XMLHttpRequest.h (right): https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.h:164: IsWithCredentials = 1 << 1, Maybe just "WithCredentials"? https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.h:170: // 1 bit remainig. If added more then increase the size of m_xhrFlags. typo: "remaining" https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.h:173: inline bool getFlag (XMLHttpRequestFlags mask) const { return m_xhrFlags & mask; } nit: extra space after getFlag. "inline" is redundant here and for other inlined methods below. https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.h:236: u_int8_t m_xhrFlags; I think we usually use "uint8_t".
https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... File Source/core/xml/XMLHttpRequest.h (right): https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.h:236: u_int8_t m_xhrFlags; On 2014/06/21 00:40:41, Chris Dumez wrote: > I think we usually use "uint8_t". Also, considering the small number of flags, I think we could have simply moved the bool members next to each other like this: bool m_async : 1; bool m_includeCredentials : 1; bool m_createdDocument : 1; ... This way, there was no need to refactor the code at all.
Thanks Chris for review! I shall upload a new patch this weekend. https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... File Source/core/xml/XMLHttpRequest.h (right): https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.h:236: u_int8_t m_xhrFlags; On 2014/06/21 00:51:20, OOO until June 25 wrote: > On 2014/06/21 00:40:41, Chris Dumez wrote: > > I think we usually use "uint8_t". > > Also, considering the small number of flags, I think we could have simply moved > the bool members next to each other like this: > bool m_async : 1; > bool m_includeCredentials : 1; > bool m_createdDocument : 1; > ... > > This way, there was no need to refactor the code at all. As flags are less than 8, I think you are right. I shall try this approach.
On 2014/06/21 01:58:41, maheshkk wrote: > Thanks Chris for review! I shall upload a new patch this weekend. > > https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... > File Source/core/xml/XMLHttpRequest.h (right): > > https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... > Source/core/xml/XMLHttpRequest.h:236: u_int8_t m_xhrFlags; > On 2014/06/21 00:51:20, OOO until June 25 wrote: > > On 2014/06/21 00:40:41, Chris Dumez wrote: > > > I think we usually use "uint8_t". > > > > Also, considering the small number of flags, I think we could have simply > moved > > the bool members next to each other like this: > > bool m_async : 1; > > bool m_includeCredentials : 1; > > bool m_createdDocument : 1; > > ... > > > > This way, there was no need to refactor the code at all. > > As flags are less than 8, I think you are right. I shall try this approach. To be clear, I am still not convinced that we should do this for XMLHttpRequest (despite me reviewing the change) as I don't believe we keep a lot of those in memory. Using bitfields has a cost in terms in performance so it should only be used when the memory benefits are important IMHO.
On 2014/06/22 13:06:09, OOO until June 25 wrote: > On 2014/06/21 01:58:41, maheshkk wrote: > > Thanks Chris for review! I shall upload a new patch this weekend. > > > > > https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... > > File Source/core/xml/XMLHttpRequest.h (right): > > > > > https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... > > Source/core/xml/XMLHttpRequest.h:236: u_int8_t m_xhrFlags; > > On 2014/06/21 00:51:20, OOO until June 25 wrote: > > > On 2014/06/21 00:40:41, Chris Dumez wrote: > > > > I think we usually use "uint8_t". > > > > > > Also, considering the small number of flags, I think we could have simply > > moved > > > the bool members next to each other like this: > > > bool m_async : 1; > > > bool m_includeCredentials : 1; > > > bool m_createdDocument : 1; > > > ... > > > > > > This way, there was no need to refactor the code at all. > > > > As flags are less than 8, I think you are right. I shall try this approach. > > To be clear, I am still not convinced that we should do this for XMLHttpRequest > (despite me reviewing the change) as I don't believe we keep a lot of those in > memory. Using bitfields has a cost in terms in performance so it should only be > used when the memory benefits are important IMHO. Chris, I thou
On 2014/06/22 13:06:09, OOO until June 25 wrote: > On 2014/06/21 01:58:41, maheshkk wrote: > > Thanks Chris for review! I shall upload a new patch this weekend. > > > > > https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... > > File Source/core/xml/XMLHttpRequest.h (right): > > > > > https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... > > Source/core/xml/XMLHttpRequest.h:236: u_int8_t m_xhrFlags; > > On 2014/06/21 00:51:20, OOO until June 25 wrote: > > > On 2014/06/21 00:40:41, Chris Dumez wrote: > > > > I think we usually use "uint8_t". > > > > > > Also, considering the small number of flags, I think we could have simply > > moved > > > the bool members next to each other like this: > > > bool m_async : 1; > > > bool m_includeCredentials : 1; > > > bool m_createdDocument : 1; > > > ... > > > > > > This way, there was no need to refactor the code at all. > > > > As flags are less than 8, I think you are right. I shall try this approach. > > To be clear, I am still not convinced that we should do this for XMLHttpRequest > (despite me reviewing the change) as I don't believe we keep a lot of those in > memory. Using bitfields has a cost in terms in performance so it should only be > used when the memory benefits are important IMHO. Sorry that comment got submitted by mistake. Yes, I agree with you about the lifetime of these objects, I thought about the same before uploading the CL, that probably these objects aren't kept for long in memory anyway. But then I thought any memory saved is memory saved, and I measured performance for the first CL by running perf tests for XHR and did not notice any difference. I saw too many bool variables in that class too that got me motivated for the first CL I uploaded. Will do the same for bitfields and will upload if there are no side effects. Then you reviewers can decide if this isnt necessary and if so will close the CL.
Chris, so like you mentioned, using bitfields instead of bitmasks has same memory benefits but had bad impact on performance. However using bit-masks as my original approach had no measurable performance impact. Numbers in XHR perf test were pretty even. New CL addresses your comments on original patch. Let me know if this CL not worth perusing, then I will close it. PTAL. https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.cpp:183: m_xhrFlags = IsAsync | IsUploadEventsAllowed | IsSameOriginRequest; On 2014/06/21 00:40:41, OOO until June 25 wrote: > Why isn't this done in the initializer list? Done. https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... File Source/core/xml/XMLHttpRequest.h (right): https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.h:164: IsWithCredentials = 1 << 1, On 2014/06/21 00:40:41, OOO until June 25 wrote: > Maybe just "WithCredentials"? Done. https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.h:170: // 1 bit remainig. If added more then increase the size of m_xhrFlags. On 2014/06/21 00:40:41, OOO until June 25 wrote: > typo: "remaining" Done. https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.h:173: inline bool getFlag (XMLHttpRequestFlags mask) const { return m_xhrFlags & mask; } On 2014/06/21 00:40:41, OOO until June 25 wrote: > nit: extra space after getFlag. "inline" is redundant here and for other inlined > methods below. Done. https://codereview.chromium.org/346033003/diff/1/Source/core/xml/XMLHttpReque... Source/core/xml/XMLHttpRequest.h:236: u_int8_t m_xhrFlags; On 2014/06/21 00:40:41, OOO until June 25 wrote: > I think we usually use "uint8_t". Done.
If you're going to do this, please just use a bitfield. bool m_foo : 1; bool m_bar : 1;
How many XHR requests do we ever have? I dobut we ever have enough that 16 bytes of memory is worth saving. Or even if we did, I suspect that XHR requests have a bunch of other objects they keep alive which weigh *way* more than 16 bytes.
Writing your class declarations with bools last to reduce padding is not a bad convention to follow if it doesn't hamper readability by splitting up fields by type. If you want to do some minor code hygiene change here, I wouldn't mind that.
On 2014/06/24 08:51:47, sof wrote: > Writing your class declarations with bools last to reduce padding is not a bad > convention to follow if it doesn't hamper readability by splitting up fields by > type. If you want to do some minor code hygiene change here, I wouldn't mind > that. sof, I have moved all bool's declarations to end of class and this still saves 16bytes with minimal code change. PTAL.
lgtm
The CQ bit was checked by mahesh.kk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mahesh.kk@samsung.com/346033003/40002
Message was sent while issue was closed.
Change committed as 176859 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
