Closed Bug 921130 Opened 6 years ago Closed 6 years ago

Minimize the #includes in js/src/jit

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: ehsan, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Comment on attachment 810705 [details] [diff] [review]
Patch (v1)

Thanks!  One nit throughout the patch: can you indent everywhere you put #idefs like so:

#ifdef MOZ_INSTRUMENTS
# include "devtools/Instruments.h"
#endif
Attachment #810705 - Flags: review?(luke) → review+
(In reply to comment #2)
> Thanks!  One nit throughout the patch: can you indent everywhere you put #idefs
> like so:
> 
> #ifdef MOZ_INSTRUMENTS
> # include "devtools/Instruments.h"
> #endif

Sure, will do before landing.
This broke no-ion builds: <https://tbpl.mozilla.org/php/getParsedLog.php?id=28432259&tree=Mozilla-Inbound>

I tried to fix this locally, but I don't think I know enough about what should and should not be in no-ion builds.  Luke, any idea who can fix those builds?
Flags: needinfo?(luke)
It's weird that this didn't show up in the try push.  You should be able to repro the compile errors locally by just configuring and building a js shell with --disable-ion.
Flags: needinfo?(luke)
https://hg.mozilla.org/mozilla-central/rev/5b35eb07b456
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 921437
Note: I am seeing a lot of build errors with --disable-ion.
Depends on: 921584
(In reply to Luke Wagner [:luke] from comment #6)
> It's weird that this didn't show up in the try push.  You should be able to
> repro the compile errors locally by just configuring and building a js shell
> with --disable-ion.

Yeah I can *reproduce* the errors just fine.  I just don't know how to fix them, because it actually requires one to know what code should be compiled in no-ion mode and which code should not... :/
Flags: needinfo?(luke)
I don't know why ForkJoin.h needs MIR.h and Clang doesn't know either (tried both debug/opt builds).
Attachment #811901 - Flags: review?(luke)
Attachment #811901 - Flags: review?(luke) → review+
Sorry for that. We have to forward-declare MDefinition, don't know how I missed that and why it only breaks Android.

I sent an updated patch to Try. On the bright side, it did fix the No-Ion build...
Let's try this again with a forward declaration of MDefinition:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e43f1bac03c1

Linux/Windows/Android/Android NoIon are all green on Try:

https://tbpl.mozilla.org/?tree=Try&showall=1&rev=1a0bf267064e
Depends on: 923354
You need to log in before you can comment on or make changes to this bug.