Summary: | Numerous performance improvements to new FastMalloc to fix regression | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Maciej Stachowiak <mjs> | ||||||
Component: | WebKit Misc. | Assignee: | Maciej Stachowiak <mjs> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Major | ||||||||
Priority: | P2 | ||||||||
Version: | 420+ | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
Attachments: |
|
Description
Maciej Stachowiak
2005-10-02 21:22:37 PDT
Created attachment 4165 [details]
numerous optimizations to the new malloc
Comment on attachment 4165 [details]
numerous optimizations to the new malloc
Uh, I meant "?"
Comment on attachment 4165 [details]
numerous optimizations to the new malloc
I'm puzzled by the use of "config.h" -- did we discover that was the best
practice and better than a prefix file?
Also, it seems that if we do use "config.h" it needs to be the first include in
each file, because other headers might have inline code that depends on it, but
it instead seems to be the second include in every file. Is this intentional or
perhaps a bug in the script you used to add it?
To echo a complaint you've made to me many times, it was really a pain to
search through all the boilerplate changes to the other files to find the
substantive changes to review.
I'm concerned that changes to constants in the FastMalloc.cpp source file seem
to be near comments that have not been updated. Are there any comments here
that need to be updated?
The use of __attribute__((always_inline)) is not inside an ifdef. Lets instead
do this in a more-portable way using a macro and __GNUC__ ifdef as we do with
PRIVATE_INLINE.
I suggest that PRIVATE_INLINE use a KXMLCORE prefix since it's a macro and in a
header.
Should remove the FIXME comment talking about PPC.
Is there an easy way to catch the mistake of pairing fastMalloc with plain old
free or vice versa in a build where NDEBUG is not defined? If not, we should
try to come up with something.
It sounds from the comments on webkit-dev like the prefix solution will not work for Symbian or Linux. Changes made in response to comments: - config.h is now the first file everywhere - if you don't include config.h but use new or delete, you now get a compile failure (on OS X at least) - comments updated to match constants - made an ifdef for use of ALWAYS_INLINE in the file - put prefix on PRIVATE_INLINE - removed now unneeded FIXME Created attachment 4171 [details]
addressed darin's comments
Comment on attachment 4171 [details]
addressed darin's comments
Why remove the include of config.h from fpconst.cpp? It needs the definition of
WORDS_BIGENDIAN.
Otherwise looks fine. I'll mark review+, but please consider the fpconst.cpp
thing.
This is also <rdar://problem/4283967> |