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

feature: getTerrainOfRoom, fast copy of staticTerrainData[room] #83

Merged
merged 4 commits into from Aug 28, 2018

Conversation

Projects
None yet
3 participants
@Mototroller
Contributor

Mototroller commented Mar 12, 2018

Suggested getTerrainOfRoom is very minor, but useful function: instead of 2500 calls of getTerrainAt to acquire static terrain data user now can call getTerrainOfRoom once to get copy of runtimeData.staticTerrainData[roomName] typed array.

There are TERRAIN_MASK_* constants within existing API which can be used for future analysis of this Uint8Array: distance transform, skeletonization, convolution with CostMatrices, transferring to WASM etc. IMHO, terrain data acquiring and transferring is pretty expensive now to try some advanced path/map techniques.

Simple test case to reproduce
const room = Game.rooms[Object.keys(Game.rooms)[0]];
let t = 0;

t = Game.cpu.getUsed();
const arr = Game.map.getTerrainOfRoom(room.name);
t = Game.cpu.getUsed() - t;
console.log(`getTerrainOfRoom   = ${t.toFixed(6)} CPU`);

t = Game.cpu.getUsed();
const own = new Uint8Array(2500);
for(let i = 0; i < 50; ++i)
    for(let j = 0; j < 50; ++j)
        own[i + 50*j] = Game.map.getTerrainAt(i, j, room.name).length;
t = Game.cpu.getUsed() - t;
console.log(`getTerrainAt 2500x = ${t.toFixed(6)} CPU`);

Typical performance tests:

[5:42:29 PM]getTerrainAt 2500x = 0.988422 CPU
[5:42:30 PM]getTerrainOfRoom   = 0.015738 CPU

Need reply to ensure there aren't security underwater rocks here (constructor or accessors rewriting?)

Forum thread to upvote/downvote this feature request :)

@artch

This comment has been minimized.

Contributor

artch commented Apr 13, 2018

I like this addition, it is worth a Subscription Token. It'll be deployed to the PTR on the next update.
Please do the following:

  1. rename the method to getRawTerrainBuffer;
  2. create a PR to https://github.com/screeps/docs, including a minimal usage example of working with the resulting buffer using mask constants.

@artch artch added the ptr-staging label Apr 13, 2018

@laverdet

This comment has been minimized.

Contributor

laverdet commented Apr 13, 2018

If you change the API to return a thin wrapper on top of the existing SharedArrayBuffer using a closure you can do this with 0 copies. Something like:

function makeInterface(buffer) {
  return {
    get(xx, yy) {
      return buffer[yy * 50 + xx];
    },
  };
}

That would offer you the flexibility to update the underlying structure of the terrain at a later time as well.

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 13, 2018

@laverdet , not sure I get your idea. The main benefit of suggested getRawTerrainBuffer, as I saw it, that it provides plain copy of static terrain data. Plain — means it's very lightweight and cheap to operate (cycles, convolutions, WASM sharing etc.), and copy — means it won't be a security issue of terrain modifying (buffer partial copy will take place, but it's the only and necessary copy, I guess).

Do you think it will be a problem to change underlying representation? So long as runtimeData.staticTerrainData[roomName] will be an instance of Uint8Array, this approach will work. Not sure it's possible to provide a zero-copy plain terrain data API in general case, it definitely relies on current underlying representation.

@artch, roger that, thanks, I'll do that these weekend as soon as possible, there is still important discussion above with @laverdet.

@laverdet

This comment has been minimized.

Contributor

laverdet commented Apr 13, 2018

As it stands now you're returning a plain Uint8Array to the user which means they will need to know what the underlying data looks like. Maybe in the future we decide that we want to change the underlying data structure and then everyone's code breaks. And actually changing the underlying structure is very likely since you can represent a room's terrain data as a Uint8Array(625) (see: native/rewrite-terrain.js) and right now it is Uint8Array(2500). In that case you need to some bitshifting to get at the data which I wouldn't expect the user to want to do.

A better way is to return a simple interface that takes two parameters and returns the terrain type. v8 will almost certainly inline this simple function so the overhead is minimal. You can keep the user away from the original Uint8Array (which is backed by a SharedArrayBuffer) by using a closure. Sharing with wasm isn't relevant in either case because to make this data available to wasm you still have to copy it into the WebAssembly.Memory instance.

So for example it would look something more like this for now:

        getTerrainOfRoom(roomName) {
            if(_.isObject(roomName))
                roomName = roomName.name;
            
            var array = (runtimeData.staticTerrainData || {})[roomName];
            if(!array)
                return undefined;
            return {
              get(xx, yy) {
                return array[yy * 50 + xx];
              },
            };
        },

Then the user would get back an object that they could call terrain.get(xx, yy) on. This wouldn't need to copy anything at all whereas the way the diff stands now every call to getTerrainOfRoom would copy 2.5kb of data. When someone gets around to bitpacking the terrain data we could change get(xx, yy) and the user would never know.

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 14, 2018

@laverdet, seems legit, gotta implement this and do some performance tests now. In case there won't be performance benefits compared with current getTerrainAt, this PR may be unnecessary.

One more question: do you think using TERRAIN_MASK_* constants is acceptable way to work with raw terrain (won't they be changed dramatically like supposed underlying terrain data representation)?

BTW:

every call to getTerrainOfRoom would copy 2.5kb of data

— definitely this was an intended behavior, this function shall not be called too often but once per huge analysis. I see one more interesting case when you pass TypedArray view as an optional argument to be filled by acquired terrain data — this array can be a WASM heap view (and we have single copy terrain transferring). But yeah, it can be achieved with your suggested approach as well 🤔

UPD: @laverdet, I can't find native/rewrite-terrain.js you've mentioned above, can you share a link?
I understood you point: "there are 3 terrain types, so we need 2 bits for each tile, so we can store room map as 2500 * 2 / 8 = 625 bytes array". So, TERRAIN_MASK_* becomes broken, and it becomes unclear what suggested get(xx,yy) function shall return in this case. Maybe it should be smth like new TERRAIN_TYPE_* constants (0, 1 and 2 for "plain", "swamp" and "wall" respectively)?

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 14, 2018

I've tried to combine both approaches with their advantages, something like this:

Sketch of an implementation:
getRawTerrain(roomName) {
    if(_.isObject(roomName))
        roomName = roomName.name;
    
    var array = (runtimeData.staticTerrainData || {})[roomName];
    if(!array)
        return undefined;
    
    return {
        get(x, y) {
            return array[y * 50 + x];
        },
        copy() {
            // As long as "array" is a TypedArray or has a valid copy c-tor:
            return new <TypedArray>(array);
        }
    };
}
  • get(x,y) is a lightweight accessor (suggested by @laverdet);
  • copy() returns initially suggested terrain array copy, and this method should be marked as an experimental (like current community hacks with CostMatrix._bits), because return value depends on underlying representation.

Performance profile looks like this:

Simple performance test over (100 samples average):
[9:21:39 PM]getTerrainAt 2500x   = 1.140463 CPU, equal=true
[9:21:39 PM]getTerrainOfRoom     = 0.001140 CPU, equal=true
[9:21:39 PM]raw.get(xx,yy) x2500 = 0.081294 CPU, equal=true

[9:21:40 PM]getTerrainAt 2500x   = 1.166008 CPU, equal=true
[9:21:40 PM]getTerrainOfRoom     = 0.001229 CPU, equal=true
[9:21:40 PM]raw.get(xx,yy) x2500 = 0.078419 CPU, equal=true

[9:21:41 PM]getTerrainAt 2500x   = 1.154372 CPU, equal=true
[9:21:41 PM]getTerrainOfRoom     = 0.001251 CPU, equal=true
[9:21:41 PM]raw.get(xx,yy) x2500 = 0.080584 CPU, equal=true
  • getTerrainOfRoom/raw.copy() is fastest: ~0.015 CPU/room
  • 2500 raw.get(xx,yy) is 5x slower than copy: ~0.08 CPU/room
  • 2500 getTerrainAt is 73x slower than copy: ~1.1 CPU/room
@laverdet

This comment has been minimized.

Contributor

laverdet commented Apr 15, 2018

UPD: @laverdet, I can't find native/rewrite-terrain.js you've mentioned above, can you share a link?

Sorry that was a local file, and not what I thought it was. driver/src/native/pf.h function look is the best example. It looks like this:

      uint16_t index = xx * 50 + yy;
      return 0x03 & terrain[index / 4] >> (index % 4 * 2);

Right now TERRAIN_MASK is 1, 2, and 4.. I think these are only used internally. Making new TERRAIN_WALL, TERRAIN_SWAMP, TERRAIN_PLAIN constants for users to use, like you suggested, would probably be a good idea.

return new array.constructor(array);

This is dangerous. We have to be super careful with these Uint8Array instances because they point to shared memory. A tricky user could do something like:

Uint8Array.prototype.constructor = function(array) {
  global.sharedBuffer = array.buffer;
  return {};
};

and then they would be able to corrupt the terrain data for all users. What you have to do is grab a reference to Uint8Array from within makeMap and then use that as the ctor. Then the user won't be able to trick the runtime into calling any evil functions.

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 15, 2018

look is the best example. It looks like this: ...

Yeah, thanks. As I understand, it should be 0x03 & (terrain[index/4] >> (index % 4 * 2)) due to precedence and it's about mentioned 2 bits extracting.

Right now TERRAIN_MASK is 1, 2, and 4. I think these are only used internally. Making new TERRAIN_[WALL/SWAMP/PLAIN] constants for users to use would probably be a good idea.

TERRAIN_MASK_LAVA in unused for now, and it's nice that other masks are 1 and 2, so it's possible to make TERRAIN_[PLAIN/WALL/SWAMP] constants equal to 0/1/2 respectively and not break backward compatibility.

I've modified implementation sketch and gonna commit it to this PR branch:

Implementation:
let terrainConstructor = null;

exports.makeMap = function(runtimeData, register) {
    terrainConstructor || (()=>{
        for(var roomName in runtimeData.staticTerrainData) {
            var array = runtimeData.staticTerrainData[roomName];
            terrainConstructor = array.constructor;
            break;
        }
    })();
    
    /* === */
    
    return {
        
        /* === */
        
        getRawTerrain(roomName) {
            if(_.isObject(roomName))
                roomName = roomName.name;
            
            var array = (runtimeData.staticTerrainData || {})[roomName];
            if(!array)
                return undefined;
            
            return {
                get(x, y) {
                    // TODO: TERRAIN_* constants?
                    var value = array[y * 50 + x];
                    return  (value & C.TERRAIN_MASK_WALL ) ? C.TERRAIN_MASK_WALL  :
                            (value & C.TERRAIN_MASK_SWAMP) ? C.TERRAIN_MASK_SWAMP : 0;
                },
                copy() {
                    return new terrainConstructor(array);
                }
            };
        },
        
        /* === */
    };
}

Performance results are pretty same, so it looks like ok.

Is it ok to "use strict" inside at least newly added API functions? If so, I'll use let and const here.

@artch

This comment has been minimized.

Contributor

artch commented Apr 16, 2018

What concernes me in this implementation is the lack of prototype in the returned object. Plain objects with methods are usually not a good idea. What if we add a new Room.Terrain prototype? Then we could make a Room.prototype.terrain getter property that internally calls Game.map.getRoomTerrain(this.name), which is an easier to use option for rooms with visibility.

Also, I didn't read all the discussion carefully, what is the reasoning behind new TERRAIN_* constants?

@artch

This comment has been minimized.

Contributor

artch commented Apr 16, 2018

Also, I think that more performant Room.Terrain utility methods may deprecate LOOK_TERRAIN stuff, which always looked a bit off. Without supporting terrain in look* methods we could simplify the output format:

lookAt

before:

[ { type: 'creep', creep: Creep },
  { type: 'structure', structure: StructureContainer },
  { type: 'terrain', terrain: 'plain' } ]

after:

[ Creep, StructureContainer ]

lookAtArea

before:

{
    10: {
        5: [{ type: 'creep', creep: Creep },
            { type: 'terrain', terrain: 'swamp' }],
        6: [{ type: 'terrain', terrain: 'swamp' }],
        7: [{ type: 'terrain', terrain: 'swamp' }]
    },
    11: {
        5: [{ type: 'terrain', terrain: 'plain' }],
        6: [{ type: 'structure', structure: StructureController },
            { type: 'terrain', terrain: 'swamp' }],
        7: [{ type: 'terrain', terrain: 'wall' }]
    }
}

after:

{
  10: {
    5: [Creep],
    6: [],
    7: []
  }
  11: {
    5: [],
    6: [StructureController],
    7: []
  }
}

This is a breaking change though, we have to think about proper deprecation process.

@laverdet

This comment has been minimized.

Contributor

laverdet commented Apr 16, 2018

What concernes me in this implementation is the lack of prototype in the returned object.

Well, you could always add one later without any breaking changes. I personally don't think it's terribly important to formalize the prototype as long as the makeshift return value has a consistent interface. That's just a matter of opinion though. There is also the fact that since the original buffer is hidden in a closure creating a formal Room.Terrain object is difficult. In that case what this would end up looking like is this:

getRawTerrain(roomName) {
// ...
let ret = new Room.Terrain();
ret.get = function(x, y) {
  var value = array[y * 50 + x];
  return  (value & C.TERRAIN_MASK_WALL ) ? C.TERRAIN_MASK_WALL  :
    (value & C.TERRAIN_MASK_SWAMP) ? C.TERRAIN_MASK_SWAMP : 0;
};
ret.copy = function() {
  return new terrainConstructor(array);
};
return ret;
}

Alternatively you could hide that logic in the constructor itself but regardless each instance created will need its own copy of each function that accesses the underlying array.

Also, I didn't read all the discussion carefully, what is the reasoning behing new TERRAIN_* constants?

MASK implies that there could be more than one terrain type. For example, TERRAIN_MASK_SWAMP | TERRAIN_MASK_WALL but this function returns a single terrain type which is not actually a mask. I think internally you can have a wall on top of a swamp which the renderer uses to smooth out the swamp layer but the user almost certainly does not care about that. There is also no existing TERRAIN_MASK_PLAIN constant because that would just be 0 and you can't have a mask of 0. Since there's only 3 terrain types the constants end up being the same but its a matter of correctness.

@artch

This comment has been minimized.

Contributor

artch commented Apr 16, 2018

@laverdet

Well, since prototypes can make things a little bit faster if all instances re-use the same method, it is pointless to add prototypes while overwriting methods in each instance manually. So the purpose is to figure out how to avoid that.

What if the get method used runtimeData.staticTerrainData directly? The performance would be worse, but how much worse?

Map.getRoomTerrain(roomName) {
  return new Room.Terrain(roomName);
}
Room.Terrain = function(roomName) {
  if(!runtimeData.staticTerrainData[roomName]) {
    throw new Error(`Could not access room ${roomName}`);
  }
  this.roomName = roomName;
}
Room.Terrain.prototype.get = function (x, y) {
  var value = runtimeData.staticTerrainData[this.roomName][y * 50 + x];
  return  (value & C.TERRAIN_MASK_WALL ) ? C.TERRAIN_MASK_WALL  :
    (value & C.TERRAIN_MASK_SWAMP) ? C.TERRAIN_MASK_SWAMP : 0;    
}
@laverdet

This comment has been minimized.

Contributor

laverdet commented Apr 16, 2018

In this case I strongly believe that a closure will be faster overall. Since JS doesn't have private fields this is a commonly-used pattern to achieve the same result. Crockford has recommended this approach https://crockford.com/javascript/private.html since the 90's and it's still the best we've got.

@artch

This comment has been minimized.

Contributor

artch commented Apr 16, 2018

We do have a closure here though, the runtimeData private closure which is accessed by Room.Terrain::get instead of Map.getRoomTerrain.

@Mototroller Could you please do your benchmarks for the prototype approach?

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 16, 2018

@artch, yep, I'll test all mentioned approaches, gonna do more careful and consistent performance benchmarks with percentiles statistics.

Btw, I've just realized that we already have pretty suitable prototype for both terrain data and paths costs — it's PathFinder.CostMatrix. If we already moved away from the idea of raw-raw data copy, what about to construct and return CostMatrix filled with appropriate TERRAIN_MASK_*/TERRAIN_* values? I'll test it's performance as well.

@artch

This comment has been minimized.

Contributor

artch commented Apr 16, 2018

I don't think PathFinder.CostMatrix is the proper abstraction here, it just doesn't feel right.

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 16, 2018

@artch, while implementing I've found that it seems like not a good idea to bind Terrain to Room objects, because room construction requires a vision, and getTerrainAt doesn't. So it looks better to make a new global Terrain prototype, like you wish, and, if necessary, just add method like Room.prototype.getTerrain() returning Terrain object with its accessors, as well as Game.map.getRoomTerrain(roomName).

UPD: nwm, I missed the idea the Room.Terrain is static constructor, so it should be ok.

@artch

This comment has been minimized.

Contributor

artch commented Apr 16, 2018

UPD: nwm, I missed the idea the Room.Terrain is static constructor, so it should be ok.

Yes, Room.Terrain is similar to StructureSpawn.Spawning, you can refer to its implementation if you wish.

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 16, 2018

Thanks, exactly, or like PathFinder.CostMatrix. I'll follow this common "wrapFn cascade" way to implement Terrain and will check its performance.

@artch

This comment has been minimized.

Contributor

artch commented Apr 16, 2018

Note that wrapFn implementation is empty in isolated-vm branch and will be most likely removed.

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 17, 2018

@artch, it's very nice, I've found and checked this wrapFn adds tons of overhead for one-liners which called very often (like get/set).

I've done some interesting performance tests.


Implementations:

Prototypes:

Room.Terrain1 = function(roomName) { "use strict";
    roomName = "" + roomName;
    if(!runtimeData.staticTerrainData[roomName])
        throw new Error(`Could not access room ${roomName}`);
    this.roomName = roomName;
};

Room.Terrain1.prototype.get = function(x, y) { "use strict";
    const value = runtimeData.staticTerrainData[this.roomName][y * 50 + x];
    return (value & C.TERRAIN_MASK_WALL) || (value & C.TERRAIN_MASK_SWAMP) || 0;
};

Closures:

Room.Terrain2 = function(roomName) { "use strict";
    roomName = "" + roomName;
    
    const array = (runtimeData.staticTerrainData || {})[roomName];
    if(!array)
        throw new Error(`Could not access room ${roomName}`);
    
    this.get = ((x,y)=>{
        const value = array[y * 50 + x];
        return (value & C.TERRAIN_MASK_WALL) || (value & C.TERRAIN_MASK_SWAMP) || 0;
    });
};

Results:
CPU (50, 75, 95, 99 percentiles over 200 samples), two consecutive ticks:

getTerrainAt 2500x (DEFAULT)             =    || 0.9564     | 1.0243     | 1.2420     | 1.3322     ||
TypedArray clone                         =    || 0.0017     | 0.0020     | 0.0033     | 0.0125     ||
Terrain.get(xx,yy) x2500 (PROTO)         =    || 0.1859     | 0.1932     | 0.2554     | 0.3025     ||
Terrain.get(xx,yy) x2500 (CLOSURE)       =    || 0.0566     | 0.0631     | 0.0831     | 0.1250     ||

getTerrainAt 2500x (DEFAULT)             =    || 0.9002     | 0.9614     | 1.0386     | 1.1357     ||
TypedArray clone                         =    || 0.0013     | 0.0014     | 0.0049     | 0.0078     ||
Terrain.get(xx,yy) x2500 (PROTO)         =    || 0.1823     | 0.1924     | 0.2113     | 0.2610     ||
Terrain.get(xx,yy) x2500 (CLOSURE)       =    || 0.0599     | 0.0633     | 0.0789     | 0.0982     ||

As @laverdet pointed out above, closures are faster than prototypes (due to fewer levels of indirection).

So, if you wish and consider it's worth to implement, closures variant (Terrain2) looks pretty good.

@artch

This comment has been minimized.

Contributor

artch commented Apr 18, 2018

Very well, let's go for Terrain2 then.

@artch, it's very nice, I've found and checked this wrapFn adds tons of overhead for one-liners which called very often (like get/set).

Unfortunately, in non-ivm branch it's still needed to prevent hacks of function constructors.

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 18, 2018

@artch, so, what way it should be implemented for now? Benchmarks have been made for getters without wrapFn wrappers. When I added them (like .get = wrapFn(function(x,y){...})), performance became much more worse:

`get` with `wrapFn` performance:
getTerrainAt 2500x (DEFAULT)             =    || 0.9205     | 0.9728     | 1.0699     | 1.1642     ||
TypedArray clone                         =    || 0.0026     | 0.0046     | 0.0140     | 0.0712     ||
Terrain.get(xx,yy) x2500 (PROTO)         =    || 0.7896     | 0.8741     | 1.0018     | 3.8844     ||
Terrain.get(xx,yy) x2500 (CLOSURE)       =    || 0.5555     | 0.6133     | 0.7836     | 1.1663     ||
.

But, as I see, map.getTerrainAt isn't wrapped by wrapFn. I dunno what exact security case it should solve (can imagine it's smth about new <Constructor>() and arguments interception), so is it really needed to wrap Terrain2 getter here (constructor wrapping is fine)? So long it's not a prototype method but "free" closure, it should be ok I guess, even when Babel transpile this get lambda to ordinary function(x,y){...}.

@Mototroller Mototroller referenced this pull request Apr 18, 2018

Merged

feat: Room.Terrain docs #95

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 18, 2018

I've created a PR to docs. Please check it for consistency with you suggestions when it's convenient.

Overview of current PR state, as I see it:

  • new prototype Room.Terrain
  • constructor Terrain(roomName)
  • getter Terrain.get(x, y), closure
  • getter Terrain.getRawBuffer([dstArray]), uses copy c-tor or TypedArray.set(), marked as experimental in docs
  • static map function Game.map.getRoomTerrain(roomName)

UPD: signatures had been changed (bold font).

@laverdet

This comment has been minimized.

Contributor

laverdet commented Apr 18, 2018

If your goal with getRawBuffer is fast access in wasm you may want to consider accepting an instance of Uint8Array as an optional parameter that way it can copy directly into wasm memory.

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 18, 2018

@laverdet, I thought about that, but TypedArray.set() (which will be used to copy data) can be easily overwritten, so user will get access to its argument, and it is private Uint8Array points to SharedArrayBuffer, woops.

But TypedArray.prototype.set() can be saved before tick initialization as well as TypedArray constructor 🤔 I think, it sounds good, and it will remove one unnecessary copy.

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 19, 2018

Terrain implementation commited to be reviewed and approved. Method getRawBuffer now have optional destinationArray parameter which, if present, will be filled by TypedArray.set(<raw_buffer>) function (saved in the module as well as <terrainArray>.constructor, see sources).

@artch, still cannot understand why some methods are wrapped by wrapFn and others don't. Current commited implementation uses wrapFn for get method, but it's very bad for performance (see measurements). getRawBuffer() works fine and as intended, but get() doesn't, 2500x calls of wrapped get() cost ~0.6 CPU (when 2500x getTerrainAt() cost ~1.0 CPU).

Btw, what da hack is wrong with conflicts? Not sure if I did smth wrong, just created and pushed a new commit to my forked getTerrainOfRoom branch. UPD: got it, there was a global prototypes fix and one line has been changed. I think it can be resolved with upcoming PR commits or by Github GUI.


I found that some prototypes and constructors within engine modules scope are differ from players sandbox scope ones. Is it intended behavior? When I tried to check type of an getRawBuffer argument, it fails like:

(dstArray instanceof Uint8Array) === false,

even when dstArray = new Uint8Array(2500) and toString() === "function Uint8Array(){[native code]}". I definitely f*ked up by all this security fiesta and confused about which functions should be wrapped, which should be saved, frozen, etc. 😕

@laverdet

This comment has been minimized.

Contributor

laverdet commented Apr 19, 2018

still cannot understand why some methods are wrapped by wrapFn and others don't. Current commited implementation uses wrapFn for get method

He explained this earlier on in the ticket though not super well. wrapFn was needed before isolated-vm. You could do something like:

var require = Game.creeps.John.move.constructor.prototype.prototype.require;
require('fs').writeFileSync(...)

When running without isolated-vm the function looks like this:

var getSandboxedFunctionWrapperScript = new vm.Script("(function sandboxedFunctionWrapper(fn) { return function() { return fn.apply(this, arguments); } })");
register.wrapFn = getSandboxedFunctionWrapperScript.runInContext(globals);

But in isolated-vm it's replaced with the simple passthrough that you see today. I think that you can remove the call to this function but you'll have to get Artem's word on that.

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Apr 20, 2018

@laverdet, thanks, I tried some common approaches already like you mentioned above, but I didn't succeed, even when I didn't wrap Terrain/get/getRawBuffer with wrapFn. Maybe that was indirectly fixed as well as some other issue, maybe I didn't go deep enough 🙂

Anyway, yeah, it's needed to be reviewed by Artem, it's really very easy to miss something important here.

TerrainConstructorSet.call(destinationArray, array);
return destinationArray;
}
return new TerrainConstructor(array);

This comment has been minimized.

@Mototroller

Mototroller Apr 20, 2018

Contributor

I think, TerrainConstructor and TerrainConstructorSet need to be wrapped with wrapFn as well, but not sure. Smth like:

return new (register.wrapFn(TerrainConstructor)(array));

@artch artch merged commit 65fe4eb into screeps:ptr Aug 28, 2018

@artch

This comment has been minimized.

Contributor

artch commented Aug 28, 2018

As this PR has been merged to the PTR, Mototroller receives 5 Subscription Tokens. Thanks for the contribution!

One question regarding this:

Current commited implementation uses wrapFn for get method, but it's very bad for performance (see measurements). getRawBuffer() works fine and as intended, but get() doesn't, 2500x calls of wrapped get() cost ~0.6 CPU (when 2500x getTerrainAt() cost ~1.0 CPU).

Is this benchmarked on isolated VM or shared VM?

@Mototroller

This comment has been minimized.

Contributor

Mototroller commented Aug 28, 2018

Thanks!

Is this benchamrked on isolated VM or shared VM?

@artch, IIRC, it tested on isolated VM, but I'm not sure. I gonna make some new tests and measurements later this week due to lots of major improvements have been made since last time I played with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment