Bug 68567 - Add custom vtable struct to ClassInfo struct
Summary: Add custom vtable struct to ClassInfo struct
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on: 68404
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-21 13:21 PDT by Mark Hahnenberg
Modified: 2011-09-26 00:04 PDT (History)
4 users (show)

See Also:


Attachments
Patch (79.32 KB, patch)
2011-09-23 14:49 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fixing review issues (80.10 KB, patch)
2011-09-23 17:25 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2011-09-21 13:21:29 PDT
We want to put a vtable in ClassInfo in preparation for moving virtual methods out of JSCell and into our own custom vtable representation.  To concisely generate this table in each file that defines its own ClassInfo struct (s_info), we could use a macro called CREATE_METHOD_TABLE that takes the name of the class as the argument and generates a statically declared struct that contains all of the pointers to the static methods that represent the virtual functions for that particular class.

In this patch, we will also include the first virtual method to be included in the table, visitChildren.
Comment 1 Mark Hahnenberg 2011-09-23 14:49:45 PDT
Created attachment 108542 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-23 16:00:51 PDT
Comment on attachment 108542 [details]
Patch

Attachment 108542 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9842031

New failing tests:
svg/custom/svg-fonts-word-spacing.html
Comment 3 Adam Barth 2011-09-23 16:07:30 PDT
> svg/custom/svg-fonts-word-spacing.html

Sorry, this test seems to be super flaky.
Comment 4 Mark Hahnenberg 2011-09-23 16:08:42 PDT
(In reply to comment #3)
> > svg/custom/svg-fonts-word-spacing.html
> 
> Sorry, this test seems to be super flaky.

:-(  Thanks for letting me know it's (most likely) not my fault.
Comment 5 Geoffrey Garen 2011-09-23 17:08:55 PDT
Comment on attachment 108542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108542&action=review

r=me with the changes I suggested.

> Source/JavaScriptCore/runtime/ClassInfo.h:38
> +    struct VirtualMethodTable {
> +        typedef void (*VisitChildrenFunctionPtr)(JSCell*, SlotVisitor&);
> +        VisitChildrenFunctionPtr visitChildrenFunctionPtr;
> +    };
> +
> +#define CREATE_METHOD_TABLE(ClassName) { \

It is bad when one thing becomes two. You call this thing "virtual method table" and "method table". (Below, you also call it "vtable".) Let's call it exactly one thing everywhere. I like "method table".

> Source/JavaScriptCore/runtime/RegExpObject.h:85
>          virtual void visitChildrenVirtual(SlotVisitor&);
>          static void visitChildren(JSCell*, SlotVisitor&);

Why did these need to become public? This is the kind of thing that can benefit from a line-by-line ChangeLog comment. You should add one.
Comment 6 Mark Hahnenberg 2011-09-23 17:25:36 PDT
Created attachment 108566 [details]
Fixing review issues
Comment 7 Oliver Hunt 2011-09-23 18:08:33 PDT
Comment on attachment 108566 [details]
Fixing review issues

r=me
Comment 8 WebKit Review Bot 2011-09-26 00:04:38 PDT
Comment on attachment 108566 [details]
Fixing review issues

Clearing flags on attachment: 108566

Committed r95936: <http://trac.webkit.org/changeset/95936>
Comment 9 WebKit Review Bot 2011-09-26 00:04:43 PDT
All reviewed patches have been landed.  Closing bug.