Bug 124761 - Combine SymbolTable and SharedSymbolTable
Summary: Combine SymbolTable and SharedSymbolTable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 124760
  Show dependency treegraph
 
Reported: 2013-11-21 21:30 PST by Filip Pizlo
Modified: 2013-11-22 14:01 PST (History)
14 users (show)

See Also:


Attachments
the patch (20.95 KB, patch)
2013-11-21 23:22 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (21.61 KB, patch)
2013-11-21 23:26 PST, Filip Pizlo
ggaren: review-
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
the patch (21.80 KB, patch)
2013-11-22 12:57 PST, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2013-11-21 21:30:00 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2013-11-21 23:22:47 PST
Created attachment 217651 [details]
the patch
Comment 2 Filip Pizlo 2013-11-21 23:26:58 PST
Created attachment 217652 [details]
the patch
Comment 3 EFL EWS Bot 2013-11-21 23:37:36 PST
Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/33728010
Comment 4 Build Bot 2013-11-21 23:57:47 PST
Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/33608018
Comment 5 Build Bot 2013-11-22 00:34:21 PST
Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/33718011
Comment 6 kov's GTK+ EWS bot 2013-11-22 00:39:55 PST
Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/33618006
Comment 7 EFL EWS Bot 2013-11-22 01:02:12 PST
Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/33948001
Comment 8 Build Bot 2013-11-22 01:45:05 PST
Comment on attachment 217652 [details]
the patch

Attachment 217652 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/33778006
Comment 9 Geoffrey Garen 2013-11-22 12:29:54 PST
Comment on attachment 217652 [details]
the patch

Approach looks good to me, but build is broken:

Undefined symbols for architecture x86_64:
  "__ZNK3JSC7JSValuecvPMS0_PvEv", referenced from:
      __ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o
      __ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o
  "__ZNK3JSC7JSValue6asCellEv", referenced from:
      __ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o
      __ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o

Maybe you need to #include JSValueInlines.h?
Comment 10 Filip Pizlo 2013-11-22 12:34:10 PST
(In reply to comment #9)
> (From update of attachment 217652 [details])
> Approach looks good to me, but build is broken:
> 
> Undefined symbols for architecture x86_64:
>   "__ZNK3JSC7JSValuecvPMS0_PvEv", referenced from:
>       __ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o
>       __ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o
>   "__ZNK3JSC7JSValue6asCellEv", referenced from:
>       __ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o
>       __ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o

Yeah I have a fix locally, just haven't uploaded it yet.

> 
> Maybe you need to #include JSValueInlines.h?

We shouldn't ever include that file.  The correct thing to do is to make sure that every .cpp file includes "Operations.h", which pulls in a bunch of these BlahInlines.h files.  I had forgotten to add such an include to SymbolTable.cpp.

(I did a refactoring about a year ago that made Opertions.h work this way.  It was because Ossy and I ended up reaching the end of the line for JSC's include dependency graph - there was a refactoring for which no valid #include order in VM.h existed; one variant would break Darwin and another would break everything else.  It was impossible to work out why crap broke.  So with a bunch of help I moved a lot of inline methods into Inlines.h files, but that created a situations where we would have to include like three different BlahInlines.h files in every .cpp file.  So I instituted the "Operations.h is your friend" rule, so that if you include that, you get the inlines you need.)
Comment 11 Geoffrey Garen 2013-11-22 12:37:44 PST
> > Maybe you need to #include JSValueInlines.h?
> 
> We shouldn't ever include that file.  The correct thing to do is to make sure that every .cpp file includes "Operations.h", which pulls in a bunch of these BlahInlines.h files.

Sounds like an OK rule. This kind of rule would be easier to communicate if we had a file with a clearer name. Maybe JSCInlines.h? We could either factor out the #includes from Operations.h's into a new file, or we could rename Operations.h to JSCInlines.h.
Comment 12 Filip Pizlo 2013-11-22 12:57:44 PST
Created attachment 217710 [details]
the patch
Comment 13 Geoffrey Garen 2013-11-22 13:45:34 PST
Comment on attachment 217710 [details]
the patch

r=me
Comment 14 Filip Pizlo 2013-11-22 14:01:41 PST
Landed in http://trac.webkit.org/changeset/159713