Bug 41790 - Make FastMalloc more portable
Summary: Make FastMalloc more portable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 41840
Blocks: 52055
  Show dependency treegraph
 
Reported: 2010-07-07 12:44 PDT by Patrick R. Gansterer
Modified: 2011-01-07 06:36 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.60 KB, patch)
2010-07-07 12:45 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Some basics changes (1.99 KB, patch)
2010-07-27 04:50 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2010-07-07 12:44:55 PDT
see patch
Comment 1 Patrick R. Gansterer 2010-07-07 12:45:48 PDT
Created attachment 60766 [details]
Patch
Comment 2 WebKit Review Bot 2010-07-07 12:49:06 PDT
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.
Comment 3 Geoffrey Garen 2010-07-07 12:56:14 PDT
Comment on attachment 60766 [details]
Patch

r=me
Comment 4 WebKit Commit Bot 2010-07-08 00:38:39 PDT
Comment on attachment 60766 [details]
Patch

Clearing flags on attachment: 60766

Committed r62765: <http://trac.webkit.org/changeset/62765>
Comment 5 WebKit Commit Bot 2010-07-08 00:38:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 WebKit Review Bot 2010-07-08 00:49:47 PDT
http://trac.webkit.org/changeset/62765 might have broken Qt Linux Release
Comment 7 Csaba Osztrogonác 2010-07-08 01:45:07 PDT
(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?
Comment 8 Zoltan Herczeg 2010-07-08 02:32:36 PDT
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
Comment 9 Mark Rowe (bdash) 2010-07-08 03:53:40 PDT
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.
Comment 10 Patrick R. Gansterer 2010-07-08 05:16:36 PDT
(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?
Comment 11 Mark Rowe (bdash) 2010-07-08 22:56:03 PDT
There doesn’t appear to be any information in the bug about what the Qt problem *is*.
Comment 12 Patrick R. Gansterer 2010-07-27 04:50:49 PDT
Created attachment 62683 [details]
Some basics changes
Comment 13 WebKit Review Bot 2010-07-27 04:53:12 PDT
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.
Comment 14 Eric Seidel (no email) 2010-07-27 05:01:29 PDT
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.)
Comment 15 Patrick R. Gansterer 2010-07-27 05:05:06 PDT
(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).
Comment 16 WebKit Commit Bot 2010-08-10 13:47:37 PDT
Comment on attachment 62683 [details]
Some basics changes

Clearing flags on attachment: 62683

Committed r65091: <http://trac.webkit.org/changeset/65091>
Comment 17 WebKit Commit Bot 2010-08-10 13:47:44 PDT
All reviewed patches have been landed.  Closing bug.