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

port to spidermonkey 60 #2170

Closed
wants to merge 2 commits into from
Closed

port to spidermonkey 60 #2170

wants to merge 2 commits into from

Conversation

janl
Copy link
Member

@janl janl commented Sep 5, 2019

@jiangphcn has agreed to take this over. I’m making this a wip PR so we can communicate planning and progress to the rest of the project.

Current plan:

  • move from 52 to 60, because that fixes a number of issues existing in 52.
  • continue to migrate the API from SM185 to SM60
  • wrap functions to post-SM185 format.
    • I’d suggest we just wrap them and discard any code outside of e.g. function (doc) {} for a map function. Alternatively, we can use the transpile function from @davisp’s chakra experiment (can’t find it at the moment, maybe @davisp can help?)
  • getting all tests to pass

@davisp
Copy link
Member

davisp commented Sep 5, 2019

@janl The original transpile idea was from the couch-chakra PoC from @dmunch

https://github.com/dmunch/couch-chakra/tree/master/js

In my super limited testing it appears to work quite well though I haven't sent anything super complicated through it.

Also for anyone that works on this, one thing I had to learn the hard way is that this line is a patch from the original esprima:

https://github.com/dmunch/couch-chakra/blob/master/js/esprima.js#L3385

By default, esprima will reject our code so we have to apply the patch to not reject the anonymous function so we can tweak the AST. Bit obvious in hindsight but I remember spending a couple hours tracking that down.

@jiangphcn jiangphcn force-pushed the sm52 branch 2 times, most recently from 49b4dfc to 0c7d2b8 Compare September 19, 2019 09:35
@jiangphcn
Copy link
Contributor

hey @janl, i started from util.cpp/h to move SpiderMonkey to 60, and now there is no compile error in util.cpp. I will continue to work on main.cpp, http.cpp, and utf8.cpp, etc. And then will perform thorough test against the changes.

PS: working on several things in parallel, and so didn't work on this with full time. But I will try to hurry up.

@jiangphcn
Copy link
Contributor

hey @janl I added new commit dee80f2 to move SM to 60 for main.cpp, http.cpp, and utf8.cpp. Using this commit, codes can be compiled using SM60. Still there are a few to-dos I marked, but want to share current progress. cc @davisp @kocolosk

@jiangphcn
Copy link
Contributor

recent update is reflected in #1875 (comment)

@jiangphcn jiangphcn force-pushed the sm52 branch 13 times, most recently from 74b45a0 to 6cdbe1f Compare October 19, 2019 15:41
@jiangphcn jiangphcn force-pushed the sm52 branch 7 times, most recently from aac05cb to 6fc69e5 Compare October 30, 2019 06:20
@jiangphcn jiangphcn force-pushed the sm52 branch 6 times, most recently from 7613e7d to 6c00fb0 Compare October 31, 2019 07:06
@jiangphcn
Copy link
Contributor

hey @janl hey @davisp

After working on this task from Sep, I tried to come up with ready-for-review commit 6c00fb0 now. All eunit tests, mango tests and elixir tests passed. For Javascript tests, only one thing related to require() in design_docs.js is open. Other tests passed as well.

Based on this situation, I would like to request @davisp to review first. In parallel, I can work on possible changes for Windows platform.

Screen Shot 2019-10-31 at 7 57 01 PM

Any comments are highly welcome.

@jiangphcn jiangphcn requested a review from davisp October 31, 2019 12:08
Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

This looks pretty good overall. Though there are a few issues we need to decide on. The biggest of which is whether we want to continue to support SpiderMonkey 1.8.5 for 3.x. And if so, how we should do that. The most straight forward approach would likely be to just create two complete implementations that can be selected from at compile time. Slightly harder but involving less code duplication would be to have a compatibility header that's selected (still at ./configure time). The hardest part here would likely be in the rebar.config.script but even there I think we could create a fairly straightforward toggle between versions.

For the SpiderMonkey 60 code itself, I couldn't figure out why object sealing isn't working. The code appears to be converted correctly but the tests are still commented out. Also, I wasn't able to figure out what's blocking the require() functionality as I wouldn't have expected that to be dependent on the underlying engine.

@jiangphcn jiangphcn force-pushed the sm52 branch 7 times, most recently from dc39b96 to 84a2cbf Compare December 3, 2019 10:48
@jiangphcn jiangphcn changed the title wip: port to spidermonkey 60 port to spidermonkey 60 Dec 3, 2019
@jiangphcn
Copy link
Contributor

Thanks Paul @davisp for your comments. It is really pleasure to move forward.

I believe that it is great idea to introduce switch option to select between 1.8.5 and 6.0. I vote still to leave support of 1.8.5 for some time, especially considering there are multiple variants in windows platform to support. Thanks for your branch https://github.com/apache/couchdb/compare/sm60-davisp?expand=1. I added two additional commits today to introduce javascript for spidermonkey 60 support.

Also, I took some time to address the issue of the require() functionality and object sealing. Now we don’t need to comment out any lines for them.

cc @janl

@wohali
Copy link
Member

wohali commented Dec 4, 2019

@jiangphcn I ran out of time today but this is on my agenda for tomorrow.

@jiangphcn
Copy link
Contributor

@wohali that's fine. it is great that you can take time to look into the codes and documentation.

In general, the change happened in this PR. From this PR, you can easily see the change to support spidermonkey 60. Paul @davisp created another branch to introduce switch option to select which version of spidermonkey to build, 1.8.5 or 6.0, or others in the future. Perhaps, we can use this branch to create another PR in favor of current PR for merge. But anyway, the main logic are the same.

For Windows support, now I can compile spidermonkey 60 and Couchjs and run it. However, it needs Visual Studio 2015 or later versions. This also implied impact to other C/C++ based libraries and need more test. Also there is need to update supported Windows version for CouchDB, etc. Because we still have spidermonkey 1.8.5 support. This allows us to keep intact in Couchdb 3.0 for Windows. How about stating that spidermonkey 6.0 in Windows in CouchDB 3.0 is technology preview? We can improve it over time and give official support in CouchDB 3.x.

@jiangphcn
Copy link
Contributor

Close in favor of #2345

@jiangphcn jiangphcn closed this Dec 23, 2019
@wohali wohali deleted the sm52 branch October 21, 2020 19:16
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

Successfully merging this pull request may close these issues.

4 participants