Bug 41790 – Make FastMalloc more portable

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 60766 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/wtf/FastMalloc.cpp:418:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/wtf/FastMalloc.cpp:1447:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/FastMalloc.cpp:1448:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 2 files


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

[Geoffrey Garen]

Comment on attachment 60766 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 60766 [details]
Patch

Clearing flags on attachment: 60766

Committed r62765: <http://trac.webkit.org/changeset/62765>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62765 might have broken Qt Linux Release

[Csaba Osztrogonác]

(In reply to comment #6)
> http://trac.webkit.org/changeset/62765 might have broken Qt Linux Release

Rolled out by http://trac.webkit.org/changeset/62768 because it broke all jscore and layout tests on Qt bot.
http://build.webkit.org/builders/Qt%20Linux%20Release/builds/14904/steps/jscore-test/logs/stdio
http://build.webkit.org/results/Qt%20Linux%20Release/r62767%20(14904)

I cc-ed the fastmalloc perfect Zoltan. Could you check what was the problem?

[Zoltan Herczeg]

If we use threading.h, and wtf based mutexes, we should ALSO create the thread by wtf based method.

runScavengerThread calls the scavengerThread()

1492	void TCMalloc_PageHeap::initializeScavenger()
1493	{
1494	  m_scavengeThreadActive = true;
1495	  pthread_t thread;
1496	  pthread_create(&thread, 0, runScavengerThread, this);
1497	}

pthread_create - should be replaced by its threading.h variant

I think Qt depends on some thread specific variables, which are not created for the thread by pthread_create

I suggest to rollout this patch until we fix and test this issue

[Mark Rowe (bdash)]

ThreadSpecific.h allocates memory using FastMalloc, so it can’t be used within FastMalloc’s implementation.  It didn’t appear to be used by the attached patch anyway, so maybe that’s besides the point.

I don’t think Threading.h is the right header to include.  The data types used all seem to come from ThreadingPrimitives.h, so why not include that instead?

I’m also wary of introducing WTF types in to FastMalloc.  Many bits of code in WTF allocate memory in order to do their job, which isn’t something that’s sensible to do via the normal means from within the memory allocator.  WTF::createThread is one such function.

I’m curious why this weird subset of FastMalloc was modified in this manner.  There’s a bunch of other code in FastMalloc that uses pthreads after all.

[Patrick R. Gansterer]

(In reply to comment #9)
> ThreadSpecific.h allocates memory using FastMalloc, so it can’t be used within FastMalloc’s implementation.  It didn’t appear to be used by the attached patch anyway, so maybe that’s besides the point.
I need the following lines for WinCE and didn't wanted to copy them:
#ifndef TLS_OUT_OF_INDEXES
#define TLS_OUT_OF_INDEXES 0xffffffff
#endif

> I don’t think Threading.h is the right header to include.  The data types used all seem to come from ThreadingPrimitives.h, so why not include that instead?
> I’m also wary of introducing WTF types in to FastMalloc.  Many bits of code in WTF allocate memory in order to do their job, which isn’t something that’s sensible to do via the normal means from within the memory allocator.  WTF::createThread is one such function.
> I’m curious why this weird subset of FastMalloc was modified in this manner.  There’s a bunch of other code in FastMalloc that uses pthreads after all.
I ported the other code too to native windows, but when using WTF::createThread there is exactily this problem:
WTF::createThreadInternal (at least on windows) does a "new ThreadFunctionInvocation", which wants to be allocated via FastMalloc, and this results in an endless loop.

Is there a good way to reuse the current Mutex/ThreadCondition/ThreadSpecific stuff? Or do I need to implement it again?
Maybe we can only fix the Qt problem? What about QtWin?
I don't think that allocating Mutex and ThreadCondition via Fastalloc should be a problem? Any ideas about the exact Qt problem?

[Mark Rowe (bdash)]

There doesn’t appear to be any information in the bug about what the Qt problem *is*.

[Patrick R. Gansterer]

Created attachment 62683 [details]
Some basics changes

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/FastMalloc.cpp:3065:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/FastMalloc.cpp:3065:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 2 files


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

[Eric Seidel (no email)]

Comment on attachment 62683 [details]
Some basics changes

The changes make sense, and look fine.  But Why?  Which non-MSVC windows compiler are you trying to compile with?  I assume this is some sort of Qt build?  (Your ChangeLog should ideally say.)

[Patrick R. Gansterer]

(In reply to comment #14)
> (From update of attachment 62683 [details])
> The changes make sense, and look fine.  But Why?  Which non-MSVC windows compiler are you trying to compile with?  I assume this is some sort of Qt build?  (Your ChangeLog should ideally say.)
I try to port the FastMalloc to WinCE. The change form COMPILER(MSVC)to OS(WINDOWS) doesn't have any effect. Only cleanup (so no real reason to mention it explicit in ChangeLog).

[WebKit Commit Bot]

Comment on attachment 62683 [details]
Some basics changes

Clearing flags on attachment: 62683

Committed r65091: <http://trac.webkit.org/changeset/65091>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.