We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=9efa9bce49d30ac11cfca09aacc53afd6bb5eefa

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

1% compiler_metrics num_static_constructors windows2012-32 opt msvc 539.00 -> 543.00
1% compiler_metrics num_static_constructors windows2012-64 opt msvc 630.00 -> 634.00

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=19188

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Build_Metrics

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Regressions from comment 0 refer to parts 5 to 8 from bug 1522837.

Regressions bellow add up to upper ones. These were caused by parts 3 and 4 of same bug 1522837.

== Change summary for alert #19213 (as of Thu, 07 Feb 2019 06:04:54 GMT) ==

Regressions:

0% compiler_metrics num_static_constructors windows2012-32 opt msvc 538.00 -> 539.00
0% compiler_metrics num_static_constructors windows2012-64 opt msvc 628.00 -> 629.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19213

This is from VMFunction definitions, one static constructor for each VMFunction we added.

The JS engine has many VMFunctions and each one has a static constructor to add the VMFunction to a linked list of functions. These static initializers are pretty cheap, just a few instructions. In bug 1525674 I'll remove some other VMFunctions and that should improve the numbers again, but in general it's expected that this will fluctuate as people add/remove code.

It's hard to fix this unfortunately (people have looked into it before). Short-term we could decrease the numbers a bit by sharing duplicate VMFunctions more aggressively, but that has some downsides and the numbers will still improve/regress from time to time...

@nbp: I wonder if we could store the VMFunctions in a (constexpr and statically allocated) array instead of linked list. We would have to use some sort of macro to fill this array + define either an enum class like "FunctionFoo = 0, FunctionBar = 1," or constexpr VMFunction* pointers into the array for each of them.

What do you think? I think it's worth considering alternatives even if it makes things a bit less ergonomic.

A more concrete proposal:

(1) In a header file we have a macro "list" like this:

  _(GetProperty, js::GetProperty) \
  _(SetProperty, js::SetProperty, extra VMFunction args) \
  ...

In the header file we use the macro to define an enum:

enum class VMFunctionIndex : uint32_t { GetProperty, SetProperty, ... }

(2) In the cpp file we have a list of signatures:

  using GetPropertyFn = bool (*)(JSContext*, HandleObject, MutableHandleValue);
  using SetPropertyFn = ...
  ...

This file also uses the macro in (1) to automatically generate:

static constexpr VMFunction vmFunctions[] = {
  FunctionInfo<GetPropertyFn>(js::GetProperty, ...),
  FunctionInfo<SetPropertyFn>(js::SetProperty, ...),
  ...
};

(3) We have the following function to do the lookup:

const VMFunction& GetVMFunction(VMFunctionIndex index) { return vmFunctions[index]; }

To add a new VMFunction, you have to (1) add an entry to the macro in the header file (2) add a using statement in the cpp file. After that you can just use callVM(VMFunctionIndex::Foo).

When someone changes a function signature, they will get a compile error for FooFn in the cpp file and they can then fix all VMFunctionIndex::FooIndex uses.

Jan, do you have some updates here? Also, could you please set a priority for this bug? Thanks!

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #6)

Jan, do you have some updates here? Also, could you please set a priority for this bug? Thanks!

I'm still waiting for the needinfo from nbp.

As I mentioned above this is not a new issue and we should take the regressions from adding new VMFunctions for now, because there just isn't an alternative atm. However I know this is pretty annoying and I'm interested in fixing this soon.

I am not a fan of moving the repeated function signature far from the callVM function calls, as the idea is that one can easily check the signature of the function against the generated call site and its arguments.

Another approach would be to explicit the list which is currently being built by making a VMFunctionsList as such:

struct VMFunctionsList {
  static VMFunctionsList* last;
  VMFunctionsList* next;
  constexpr size_t length;
  constexpr VMFunction* vector;

  // variadic template ?
  VMFunctionsList(…) { … }
};

In which case, for each file defining a VMFunction we would do the following:

// At the beginning of the file.
#define VMFUNCTIONLIST

[…]

// In the middle of the file
typedef bool (*IonGetPropertyICFn)(JSContext*, HandleScript, IonGetPropertyIC*,
                                   HandleValue, HandleValue,
                                   MutableHandleValue);
static constexpr VMFunction IonGetPropertyICInfo = FunctionInfo<IonGetPropertyICFn>(
    IonGetPropertyIC::update, "IonGetPropertyIC::update");
#define VMFUNCTIONLIST  VMFUCNTIONLIST IonGetPropertyICInfo,

// At the end of the file
static constexpr VMFunction CodeGenFunctionsList*[] = [
  VMFUNCTIONLIST
  EndVMFunctionsList
];
static const VMFunctionsList CodeGenFunctions(CodeGenFunctionsList);

-- Edit: Adding missing negation in the first statement.

Another proposal for the brainstorming. (Maybe good, maybe bad...)

1. Centralize the VMFunction instances to a single jit/VMFunctions.cpp file using the constant list trick.
2. At each use site, have an |extern VMFunction<bool ()(JSContext)> FooFunc;| with explicit parameter types so JIT code can see.
3. The actual instances in VMFunctions.cpp would use decltype or whatever and don't need to copy-paste the types, only the JIT-code consumers that really care.

• Signatures probably don't need to go in any header. This will make mismatches a link error, but that is probably not too bad. We could also generate them into a header if we wanted a redefinition error instead.

• In the future, we could transparently pre-build the jit/VMFunctions.cpp file with trampolines already generated.

I'll look into fixing this next week. I agree it would be nice to have the VMFunctions defined in jit/VMFunctions but the signature near the codegen that depends on it. We can probably even move it from the global scope into the function that calls callVM.

Thanks for the ideas!

Having the function signature as part of the callVM function would actually be better than what we have today.

I guess this could be done on top of what is being suggested in comment 5, by having a Macro which generate templates for recovering the enum index given the type and function pointer:

The header would them look like:

// Ideally these value should be private and only reachable through the VMFunctionMap template.
enum class VMFunctionIndex : uint32_t {
  // Generated with the Macro
  GetProperty,
  SetProperty,
  ...
}

template <typename Function, Function fun>
struct VMFunctionMap;

// Generated with the Macro
template <>
struct VMFunctionMap<decltype(js::GetProperty), js::GetProperty> {
  static constexpr VMFunctionEnum index = VMFunctionEnum::GetProperty;
};

VMFunction& VMFunctionGet(VMFunctionEnum);

And definition of the array would be in a cpp file.

static constexpr VMFunction vmFunctions[] = {
  // Generated with the Macro.
  FunctionInfo<js::GetProperty>(js::GetProperty, ...),
  FunctionInfo<js::SetProperty>(js::SetProperty, ...),
  ...
};

VMFunction& VMFunctionGet(VMFunctionEnum e) {
  return vmFunctions[size_t(e)];
}

While the CodeGen and others would have to repeat the function type and the function pointer to make use of the VMFunction:

void CodeGenerator::visitCallGetProperty(LCallGetProperty* lir) {
  using GetPropertyFn = bool (*)(JSContext*, HandleValue, HandlePropertyName,
                                 MutableHandleValue);
  auto GetPropertyInfo =
    VMFunctionGet(VMFunctionMap<GetPropertyFn, js::GetProperty>::index);

  pushArg(ImmGCPtr(lir->mir()->name()));
  pushArg(ToValue(lir, LCallGetProperty::Value));

  callVM(GetPropertyInfo, lir);
}

I guess there are ways to make it less verbose while having an explicit mention of the type and the function pointer.

I've been trying out a few designs and I have something that works well I think. I filed bug 1530937 and will work on incrementally converting all VMFunctions.

Actually this is with MSVC, per comment 0, and we dropped support for MSVC in bug 1512504.

Bug 1530937 is worth doing anyway for various reasons and I'll keep working on it.