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

[CODE CLEANING/OPTI/BUG FIX] Improve List/Tupple/Array conversions #2299

Open
Tracked by #2250
denis-migdal opened this issue Oct 30, 2023 · 6 comments
Open
Tracked by #2250

Comments

@denis-migdal
Copy link
Contributor

denis-migdal commented Oct 30, 2023

Hi,

I'll work on that in the train.

Okay, using prototype substitution is maybe too soon for list/tupple/Array as their content will not have yet prototype substitution requiring to handle JSObj and PYObj at the same time that will kill a little performances now (as we'll have to handle 2 lists and 2 objects).

Here what I suggest for now:

  • new BrythonAPI(JSTarget, type) class, where type is the current Python representation of a type, putting a [BrythonAPI] symbol on the prototype of JSTarget. Therefore I won't have to rewrite the whole implementation of list or tupple, I'll make use of the current implementation.
  • I'll use that to add a toJSWrapper(), to start delegating responsibilities in jsobj2pyobj, and therefore cleaning it. I'll modify list and tupple class to add a [PYOBJ2JSOBJ] symbol, to start delegating responsibilities in pyobj2jsobj, and therefore cleaning it (and making it symmetric). I'll do that for other python types latter.

Now handling of list/tupple/Array, is quite inefficient as lot of copies are made, with can be error prone.
What I suggest after thinking:

  • tupple : no need for copies during conversions, internal representation should be Object.isFreeze().
  • list: send a Proxy to JS. I know they will performances, but I'm not sure performances on the JS-side matters to us (we shouldn't use it/relies on it if we stick to Brython API). This will enable to convert content when accessed/set by JS (and I think the cost of the Proxy will be mitigated by the conversions). This will enable array synchronisation without trouble, and will prevent to make copies and having different references when using JS API (e.g. storing a list inside of a JS Map). Not ideal, once we'll have prototype substitution for all types we will be able to do better.
  • Array, if Object.isFreeze() : convert to a tupple. Else, copy the values and convert to a list. It would be as if the original object change reference when modified. It isn't true, but JS internals shouldn't be known to us, so it still make sense. Not ideal, once we'll have prototype substitution for all types we will be able to do better.

Then if I have time, I may work on Brython dict and to think about it (JS <=> Brython object conversion have issues I need to think about).

Cordially

@denis-migdal
Copy link
Contributor Author

denis-migdal commented Oct 30, 2023

For dict, maybe not converting it when giving it to JS (it doesn't have a JS equivalent), and instead having a javascript.dict2JSObject() function to enable use of JS API, and raising an exception when a key isn't a string ?

That'd be a little troublesome at first, but maybe the cleaner way to proceed.

EDIT: Or use the magic solution: a proxy. Built in a way to ignore non-string keys.

Again, it'll kill performance in the JS side. But:

  • should be better than copying, and would solve some issues.
  • would allow to pass dict to JS functions without explicitly converting it with a javascript.dict2JSObject().
  • internally we can bypass the Proxy (so not loosing performances).
  • and users, if they want perf, then just copy the object before hand or use the Brython API....

@denis-migdal denis-migdal changed the title [CODE CLEANING/OPTI/BUG FIX] Improve List/Tupple internal representation [CODE CLEANING/OPTI/BUG FIX] Improve List/Tupple/Array conversions Oct 30, 2023
@denis-migdal
Copy link
Contributor Author

denis-migdal commented Oct 30, 2023

Okay, that really is a living hell xD.
Gotta clean things a little before going further.

To better understand, could you explain to me when $factory is called, and when __new__ is used ?

EDIT: Okay, I think I get it, $factory should normally call __new__ then __init__, but sometimes it doesn't to speed up the process (e.g. for the list/tuple), but it begs the question: then when __new__ is expected to be called for list and tuples ?

@denis-migdal
Copy link
Contributor Author

In js_objects.js :

if(klass === JSConstructor){
        // Instances of JSConstructor are transformed into the
        // underlying Javascript object

	throw new Error('Is it really used ?'); // I added this.

        if(pyobj.js_func !== undefined){
            return pyobj.js_func
        }
        return pyobj.js
    }

Is JSConstructor still used ?
I remember seeing some error related to JSConstructor in some pages.

@PierreQuentel
Copy link
Contributor

when new is expected to be called for list and tuples ?

It is used for code like list.__new__(list), or when creating a subclass of list.

@denis-migdal
Copy link
Contributor Author

Thanks, I pushed a PR #2300 for the work I did on the train.

I'll do another PR to fix the server CGI (I also submitted a PR to fix its listening address -with is indeed a security issue-).

@PierreQuentel
Copy link
Contributor

Is JSConstructor still used ?

Non, bien vu ! Je l'ai supprimé dans le commit ci-dessus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants