Bug 69435 – remove WTF_CHANGES define, as it is always 1

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Andy Wingo]

Hello,

I was looking through WTF today and found the WTF_CHANGES define which is always 1.  It seems that at some point some modifications were made to the malloc implementation, and the new code was under the WTF_CHANGES flag.  At some point things switched around so that old code was under #ifndef WTF_CHANGES.  That old code does not appear to be used on any platform.

This patch to be attached removes the define and related unused code.

[Andy Wingo]

Created attachment 109812 [details]
Patch

[WebKit Review Bot]

Attachment 109812 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/FastMalloc.cpp:3225:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 109812 [details]
Patch

We created this define to help us track the original version of TCMalloc. We should not remove it just because it’s always 1. If it has outlived its usefulness we could remove it, but that decision should be made by whoever is merging in new versions of TCMalloc.

[Andy Wingo]

(In reply to comment #3)
> (From update of attachment 109812 [details])
> We created this define to help us track the original version of TCMalloc. We should not remove it just because it’s always 1. If it has outlived its usefulness we could remove it, but that decision should be made by whoever is merging in new versions of TCMalloc.

Thank you for the review, Darin.  The define is poorly named, however, and I wonder if it really needs to make it all the way out to JavaScriptCore/config.h.  Also not all changes to tcmalloc are guarded by this flag.

Do you think that there is something worth doing here or should we close this bug?

[Andy Wingo]

Adding Darin to cc to close.  (I am new to WebKit, so if this is the wrong thing to do, please let me know.)

[Darin Adler]

(In reply to comment #4)
> The define is poorly named, however

I agree.

> I wonder if it really needs to make it all the way out to JavaScriptCore/config.h.

I think it’s inconvenient that it’s there. It would be nice if it wasn’t.

> Also not all changes to tcmalloc are guarded by this flag.

Also a good point.

> Do you think that there is something worth doing here or should we close this bug?

I think it’s worth finding out if we are getting any value from having our changes wrapped in #if now. If we’re not, it might be nice to reduce the noise and complexity by removing this.

I think the real question is who would know the answer to that. Who has recently dealt with comparing with the “real” TCMalloc or importing a new version of it into our source tree? I cc’d a few people who might know.

[Andy Wingo]

Thanks for the reply, Darin.

(In reply to comment #6)
> I think it’s worth finding out if we are getting any value from having our changes wrapped in #if now. If we’re not, it might be nice to reduce the noise and complexity by removing this.
> 
> I think the real question is who would know the answer to that. Who has recently dealt with comparing with the “real” TCMalloc or importing a new version of it into our source tree? I cc’d a few people who might know.

A kind ping to those people on CC for input.

Andy

[Darin Adler]

Comment on attachment 109812 [details]
Patch

Clearing review flag for now. Not sure we want to do this. Whoever is working on merging changes in from TCMalloc could help us decide.

[Csaba Osztrogonác]

Created attachment 224395 [details]
Patch

Updated patch for EWS. I'm going to send a mail to webkit-dev soon if we need it or not.

[WebKit Commit Bot]

Attachment 224395 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/FastMalloc.cpp:3738:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/FastMalloc.cpp:3809:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/FastMalloc.cpp:3873:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Csaba Osztrogonác]

Created attachment 224492 [details]
Patch

new patch for EWS - Mac buildfix included and updated to ToT

[WebKit Commit Bot]

Attachment 224492 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/FastMalloc.cpp:3738:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/FastMalloc.cpp:3809:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/FastMalloc.cpp:3868:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Csaba Osztrogonác]

TCMalloc was removed from trunk with WTF_CHANGES define too.