Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(js): Extract *info functions into individual functions #4751

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
247 changes: 207 additions & 40 deletions src/js/binaryen.js-post.js
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,25 @@ function wrapModule(module, self = {}) {
'wait64'(ptr, expected, timeout) {
return Module['_BinaryenAtomicWait'](module, ptr, expected, timeout, Module['i64']);
}
},
'module'() {
return UTF8ToString(Module['_BinaryenMemoryImportGetModule'](module));
},
'base'() {
return UTF8ToString(Module['_BinaryenMemoryImportGetBase'](module));
},
Comment on lines +713 to +718
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be on something like self['memoryImport']?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok as it is.

'initial'() {
return Module['_BinaryenMemoryGetInitial'](module);
},
'shared'() {
return Boolean(Module['_BinaryenMemoryIsShared'](module));
},
'max'() {
if (Module['_BinaryenMemoryHasMax'](module)) {
return Module['_BinaryenMemoryGetMax'](module);
} else {
return Infinity;
}
Comment on lines +713 to +730
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intended that these are mixed with the instructions like memory.fill? Seems that it's quite easy to mistake them for instructions like memory.shared. Also, in light of multi memory, there'll likely need to be a memory wrapper anyhow to deal with any memory, so I wouldn't expect these to last for very long.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see where Alon said "I think it's ok as it is." above.

I'm just making these easier to use, if this hasn't been updated for multiple memory, than someone else will need to help make those changes.

Copy link
Contributor

@dcodeIO dcodeIO Jul 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, has the following variant been considered?

Module#getMemoryModule
Module#getMemoryBase
Module#getMemoryInitial
Module#isMemoryShared
Module#hasMemoryMax
Module#getMemoryMax

Those would about match Module#setMemory for now, which is similarly subject to change, with all of them not conflicting with instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not considered anything (maybe others have) because I'm just converting what is here from "expensive grab-bag" to "individual functions". Further improvements can be done later, as @tlively mentioned in the primary thread.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this is a misunderstanding. I am not suggesting any significant changes in the above variant, just to move / rename these from memory (which so far covers exclusively instructions) to the Module instance (where methods of this kind without wrappers would typically go). Basically next to setMemory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to leave that up to @kripken since he said it was fine to have these here when I asked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good point @dcodeIO , you're right that this would mix instructions like memory.fill() with helpers. We've put the helpers on the relevant JS objects using makeExpressionWrapper. So I agree that adding new getX/setX functions alongside those is more consistent. That seems best.

@phated Sorry for not reading this more in detail the first time! (I am not as up to date on the JS API as I used to be, since I've been busy with other things...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can someone else pick up these changes? We really need these changes in Grain and I don't have time to continue to make these large changes, as I'm actively needing to interview for jobs.

}
}

Expand Down Expand Up @@ -2505,36 +2524,48 @@ function wrapModule(module, self = {}) {
};
self['getMemoryInfo'] = function() {
var memoryInfo = {
'module': UTF8ToString(Module['_BinaryenMemoryImportGetModule'](module)),
'base': UTF8ToString(Module['_BinaryenMemoryImportGetBase'](module)),
'initial': Module['_BinaryenMemoryGetInitial'](module),
'shared': Boolean(Module['_BinaryenMemoryIsShared'](module))
'module': self['memory']['module'](),
'base': self['memory']['base'](),
'initial': self['memory']['initial'](),
'shared': self['memory']['shared']()
};
if (Module['_BinaryenMemoryHasMax'](module)) {
memoryInfo['max'] = Module['_BinaryenMemoryGetMax'](module);
var max = self['memory']['max']();
if (max) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this checking? If the max is 0?

I'd expect this to check if the max is Infinity, but maybe I'm missing something...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kripken I'm not sure, I copied this from the existing code. Who wrote that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kripken how about this?

Suggested change
if (max) {
if (max !== Infinity) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I guess the NFC value here would be undefined. Apart from this, if my other suggestions are agreed upon to be considered, there will be a HasMax function around that can be used instead, closely matching the previous code.

memoryInfo['max'] = max;
}
return memoryInfo;
};
self['getNumMemorySegments'] = function() {
return Module['_BinaryenGetNumMemorySegments'](module);
};

self['memorySegment'] = {
'offset'(id) {
return Module['_BinaryenGetMemorySegmentByteOffset'](module, id);
},
'data'(id) {
const size = Module['_BinaryenGetMemorySegmentByteLength'](module, id);
const ptr = _malloc(size);
Module['_BinaryenCopyMemorySegmentData'](module, id, ptr);
const res = new Uint8Array(size);
res.set(new Uint8Array(buffer, ptr, size));
_free(ptr);
return res.buffer;
},
'passive'(id) {
return Boolean(Module['_BinaryenGetMemorySegmentPassive'](module, id));
}
}
Comment on lines +2542 to +2558
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another suggestion to make this more uniform with other code. Move / rename these to

Module#getMemorySegmentByteOffset
Module#getMemorySegmentData
Module#isMemorySegmentPassive

Then becomes

myModule.getMemorySegmentByteOffset(segmentIndex)

instead of

myModule.memorySegment.offset(id)

Apart from adhering to surrounding code style, this also has the advantage that these won't conflict with eventual setters.


self['getMemorySegmentInfoByIndex'] = function(id) {
const passive = Boolean(Module['_BinaryenGetMemorySegmentPassive'](module, id));
const passive = self['memorySegment']['passive'](id);
let offset = null;
if (!passive) {
offset = Module['_BinaryenGetMemorySegmentByteOffset'](module, id);
offset = self['memorySegment']['offset'](id)
}
return {
'offset': offset,
'data': (function(){
const size = Module['_BinaryenGetMemorySegmentByteLength'](module, id);
const ptr = _malloc(size);
Module['_BinaryenCopyMemorySegmentData'](module, id, ptr);
const res = new Uint8Array(size);
res.set(new Uint8Array(buffer, ptr, size));
_free(ptr);
return res.buffer;
})(),
'data': self['memorySegment']['data'](id),
'passive': passive
};
};
Expand Down Expand Up @@ -3227,25 +3258,25 @@ Module['expandType'] = function(ty) {
// Obtains information about a 'Function'
Module['getFunctionInfo'] = function(func) {
return {
'name': UTF8ToString(Module['_BinaryenFunctionGetName'](func)),
'module': UTF8ToString(Module['_BinaryenFunctionImportGetModule'](func)),
'base': UTF8ToString(Module['_BinaryenFunctionImportGetBase'](func)),
'params': Module['_BinaryenFunctionGetParams'](func),
'results': Module['_BinaryenFunctionGetResults'](func),
'vars': getAllNested(func, Module['_BinaryenFunctionGetNumVars'], Module['_BinaryenFunctionGetVar']),
'body': Module['_BinaryenFunctionGetBody'](func)
'name': Module['Function']['getName'](func),
'module': Module['FunctionImport']['getModule'](func),
'base': Module['FunctionImport']['getBase'](func),
'params': Module['Function']['getParams'](func),
'results': Module['Function']['getResults'](func),
'vars': getAllNested(func, Module['Function']['getNumVars'], Module['Function']['getVar']),
'body': Module['Function']['getBody'](func)
};
};

// Obtains information about a 'Global'
Module['getGlobalInfo'] = function(global) {
return {
'name': UTF8ToString(Module['_BinaryenGlobalGetName'](global)),
'module': UTF8ToString(Module['_BinaryenGlobalImportGetModule'](global)),
'base': UTF8ToString(Module['_BinaryenGlobalImportGetBase'](global)),
'type': Module['_BinaryenGlobalGetType'](global),
'mutable': Boolean(Module['_BinaryenGlobalIsMutable'](global)),
'init': Module['_BinaryenGlobalGetInitExpr'](global)
'name': Module['Global']['getName'](global),
'module': Module['GlobalImport']['getModule'](global),
'base': Module['GlobalImport']['getBase'](global),
'type': Module['Global']['getType'](global),
'mutable': Module['Global']['isMutable'](global),
'init': Module['Global']['getInitExpr'](global)
};
};

Expand All @@ -3267,17 +3298,16 @@ Module['getTableInfo'] = function(table) {
};

Module['getElementSegmentInfo'] = function(segment) {
var segmentLength = Module['_BinaryenElementSegmentGetLength'](segment);
var segmentLength = Module['ElementSegment']['getLength'](segment);
var names = new Array(segmentLength);
for (let j = 0; j !== segmentLength; ++j) {
var ptr = Module['_BinaryenElementSegmentGetData'](segment, j);
names[j] = UTF8ToString(ptr);
names[j] = Module['ElementSegment']['getData'](segment, j);
}

return {
'name': UTF8ToString(Module['_BinaryenElementSegmentGetName'](segment)),
'table': UTF8ToString(Module['_BinaryenElementSegmentGetTable'](segment)),
'offset': Module['_BinaryenElementSegmentGetOffset'](segment),
'name': Module['ElementSegment']['getName'](segment),
'table': Module['ElementSegment']['getTable'](segment),
'offset': Module['ElementSegment']['getOffset'](segment),
'data': names
}
}
Expand All @@ -3296,9 +3326,9 @@ Module['getTagInfo'] = function(tag) {
// Obtains information about an 'Export'
Module['getExportInfo'] = function(export_) {
return {
'kind': Module['_BinaryenExportGetKind'](export_),
'name': UTF8ToString(Module['_BinaryenExportGetName'](export_)),
'value': UTF8ToString(Module['_BinaryenExportGetValue'](export_))
'kind': Module['Export']['getKind'](export_),
'name': Module['Export']['getName'](export_),
'value': Module['Export']['getValue'](export_)
};
};

Expand Down Expand Up @@ -4768,8 +4798,145 @@ Module['I31Get'] = makeExpressionWrapper({
}
});

// Function wrapper
// ElementSegment wrapper
Module['ElementSegment'] = (() => {
// Closure compiler doesn't allow multiple `ElementSegment`s at top-level, so:
Comment on lines +4802 to +4803
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same :)

function ElementSegment(func) {
if (!(this instanceof ElementSegment)) {
if (!func) return null;
return new ElementSegment(func);
}
if (!func) throw Error("function reference must not be null");
this[thisPtr] = func;
}
ElementSegment['getLength'] = function(segment) {
return Module['_BinaryenElementSegmentGetLength'](segment);
};
ElementSegment['getData'] = function(segment, index) {
return UTF8ToString(Module['_BinaryenElementSegmentGetData'](segment, index));
};
ElementSegment['getName'] = function(segment) {
return UTF8ToString(Module['_BinaryenElementSegmentGetName'](segment));
};
ElementSegment['getTable'] = function(segment) {
return UTF8ToString(Module['_BinaryenElementSegmentGetTable'](segment));
};
ElementSegment['getOffset'] = function(segment) {
return Module['_BinaryenElementSegmentGetOffset'](segment);
};
deriveWrapperInstanceMembers(ElementSegment.prototype, ElementSegment);
ElementSegment.prototype['valueOf'] = function() {
return this[thisPtr];
};
return ElementSegment;
})();

// Export wrapper
Module['Export'] = (() => {
// Closure compiler doesn't allow multiple `Export`s at top-level, so:
Comment on lines +4835 to +4836
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

function Export(func) {
if (!(this instanceof Export)) {
if (!func) return null;
return new Export(func);
}
if (!func) throw Error("function reference must not be null");
this[thisPtr] = func;
}
Export['getKind'] = function(export_) {
return Module['_BinaryenExportGetKind'](export_);
};
Export['getName'] = function(export_) {
return UTF8ToString(Module['_BinaryenExportGetName'](export_));
};
Export['getValue'] = function(export_) {
return UTF8ToString(Module['_BinaryenExportGetValue'](export_));
};
deriveWrapperInstanceMembers(Export.prototype, Export);
Export.prototype['valueOf'] = function() {
return this[thisPtr];
};
return Export;
})();

// GlobalImport wrapper
Module['GlobalImport'] = (() => {
// Closure compiler doesn't allow multiple `GlobalImport`s at top-level, so:
Comment on lines +4862 to +4863
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IIFE here is not necessary. Only affects JS built-ins like Function.

function GlobalImport(func) {
if (!(this instanceof GlobalImport)) {
if (!func) return null;
return new GlobalImport(func);
}
if (!func) throw Error("function reference must not be null");
this[thisPtr] = func;
}
GlobalImport['getModule'] = function(global) {
return UTF8ToString(Module['_BinaryenGlobalImportGetModule'](global));
};
GlobalImport['getBase'] = function(global) {
return UTF8ToString(Module['_BinaryenGlobalImportGetBase'](global));
};
deriveWrapperInstanceMembers(GlobalImport.prototype, GlobalImport);
GlobalImport.prototype['valueOf'] = function() {
return this[thisPtr];
};
return GlobalImport;
})();

// Global wrapper
Module['Global'] = (() => {
// Closure compiler doesn't allow multiple `Global`s at top-level, so:
Comment on lines +4886 to +4887
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, the IIFE here is not necessary. Only affects Function.

function Global(func) {
if (!(this instanceof Global)) {
if (!func) return null;
return new Global(func);
}
if (!func) throw Error("function reference must not be null");
this[thisPtr] = func;
}
Global['getName'] = function(global) {
return UTF8ToString(Module['_BinaryenGlobalGetName'](global));
};
Global['getType'] = function(global) {
return Module['_BinaryenGlobalGetType'](global);
};
Global['isMutable'] = function(global) {
return Boolean(Module['_BinaryenGlobalIsMutable'](global));
};
Global['getInitExpr'] = function(global) {
return Module['_BinaryenGlobalGetInitExpr'](global);
};
deriveWrapperInstanceMembers(Global.prototype, Global);
Global.prototype['valueOf'] = function() {
return this[thisPtr];
};
return Global;
})();

// FunctionImport wrapper
Module['FunctionImport'] = (() => {
// Closure compiler doesn't allow multiple `FunctionImport`s at top-level, so:
Comment on lines +4916 to +4917
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As well: The IIFE here is not necessary. Only affects JS built-ins like Function.

function FunctionImport(func) {
if (!(this instanceof FunctionImport)) {
if (!func) return null;
return new FunctionImport(func);
}
if (!func) throw Error("function reference must not be null");
this[thisPtr] = func;
}
FunctionImport['getModule'] = function(func) {
return UTF8ToString(Module['_BinaryenFunctionImportGetModule'](func));
};
FunctionImport['getBase'] = function(func) {
return UTF8ToString(Module['_BinaryenFunctionImportGetBase'](func));
};
deriveWrapperInstanceMembers(FunctionImport.prototype, FunctionImport);
FunctionImport.prototype['valueOf'] = function() {
return this[thisPtr];
};
return FunctionImport;
})();

// Function wrapper
Module['Function'] = (() => {
// Closure compiler doesn't allow multiple `Function`s at top-level, so:
function Function(func) {
Expand Down