Status

()

Core
JavaScript Engine
RESOLVED INCOMPLETE
3 years ago
a year ago

People

(Reporter: Sarat Adiraj, Assigned: Sarat Adiraj)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
Created attachment 8416826 [details] [diff] [review]
v8monkey.patch

Initial patch to update v8monkey with the latest Spidermonkey build.
Thanks, Sarat! I'm sure many people will be interested in your contribution here. :)
Assignee: nobody → saratusa
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8416826 [details] [diff] [review]
v8monkey.patch

Review of attachment 8416826 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/moz.build
@@ +212,5 @@
> +    'v8-dtoa/diy-fp.cc',
> +    'v8-dtoa/fast-dtoa.cc',
> +    'v8-dtoa/platform.cc',
> +    'v8-dtoa/utils.cc',
> +    'v8-dtoa/v8-dtoa.cc',

We already have a copy of (some) dtoa files in tree at mfbt/double-conversion. Can you use those?

@@ +223,5 @@
> +    'v8api/object.cpp',
> +    'v8api/objecttemplate.cpp',
> +    'v8api/string.cpp',
> +    'v8api/template.cpp',
> +    'v8api/v8.cpp'

Did you forget to include the v8api files in this patch?
Attachment #8416826 - Flags: feedback?(till)
(Assignee)

Comment 3

3 years ago
Hi Chris,
I will add the v8api files to the patch and upload it shortly. I will check the mfbt/double-conversion stuff
Comment on attachment 8416826 [details] [diff] [review]
v8monkey.patch

Review of attachment 8416826 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, I like how tiny this patch is. If the typed array function is all you need to add to our API to be able to implement v8's on top of it, that's amazing.

Looking forward to seeing the v8api files.

::: js/src/jsfriendapi.cpp
@@ +21,5 @@
>  #include "prmjtime.h"
>  
>  #include "builtin/TestingFunctions.h"
>  #include "vm/WrapperObject.h"
> +#include "vm/TypedArrayObject.h"

This change doesn't seem to be used anymore, right?

@@ +1221,5 @@
>      return ReportIsNotFunction(cx, v);
>  }
>  
> +
> +

Nit: please remove these blank lines again

::: js/src/vm/Debugger.cpp
@@ +3365,5 @@
>          else if (entries_[targetOffset].column() != sourceColumn)
>              entries_[targetOffset] = Entry::createWithMultipleEdgesFromSingleLine(sourceLineno);
>      }
>  
> +    js::Vector<Entry> entries_;

This change shouldn't be needed

::: js/src/vm/TypedArrayObject.cpp
@@ +1234,5 @@
> +
> +
> +JS_FRIEND_API(JSObject *)
> +js_CreateTypedArrayWithBuffer(JSContext *cx, int32_t atype, JSObject *bufArg,
> +                              uint32_t byteoffset, int32_t length)

Please take a look at ArrayBufferObject::createTypedArrayFromBufferImpl. Apart from the way it extracts arguments from CallArgs, it should do what you need. If so, can you split that function into one part that extracts the correct arguments and one (non-templatized) that does the actual array construction and implement this function in terms of the second part? That should also allow you to get rid of TypedArrayConstruct above.
Attachment #8416826 - Flags: feedback?(till) → feedback+
(Assignee)

Comment 5

3 years ago
Created attachment 8417488 [details]
v8 api source code (zip)

till:  May need help generating the patch for this. I have attached the complete 
v8 source code. The v8 folders need to be placed under js/src folder
Attachment #8417488 - Flags: review+
Comment on attachment 8417488 [details]
v8 api source code (zip)

Sarat: if you are managing your patches with Mercurial's patch queue, you can "hg add js/src/v8api ; hg qrefresh" to include the v8 files in your patch itself.
Attachment #8417488 - Attachment description: v8 api source code → v8 api source code (zip)
Attachment #8417488 - Flags: review+
(Assignee)

Comment 7

3 years ago
(In reply to Chris Peterson (:cpeterson) from comment #6)
> Comment on attachment 8417488 [details]
> v8 api source code (zip)
> 
> Sarat: if you are managing your patches with Mercurial's patch queue, you
> can "hg add js/src/v8api ; hg qrefresh" to include the v8 files in your
> patch itself.

Thanks Chris,
I've attached the patch as suggested.
My initial goal was to get the original v8monkey code to work with the latest spidermonkey codebase "as-is" with as minimal changes to the spidermonkey code base as possible. 
Next steps:
I am looking through the comments and am looking into the v8-dtoa folder to see if it could be replaced with mfbt/double-conversion.
I will also look at Till's suggestion of splitting the ArrayBufferObject::createTypedArrayFromBufferImpl later on.
(Assignee)

Comment 8

3 years ago
Created attachment 8417534 [details] [diff] [review]
added v8api files to patch
Attachment #8417534 - Attachment description: added v8api files to patch. Thanks Chris ! → added v8api files to patch
Attachment #8417534 - Flags: feedback?(till)
(Assignee)

Comment 9

3 years ago
I've been making some progress on spidernode:
Any jsapi or spidermonkey engine folks in the mountain view office ? I'm looking to come over to the mountain view office to hack on it and get some help/suggestions if needed.
Sarat: you should ask on the #jsapi channel on irc.mozilla.org. All JSAPI and SpiderMonkey developers hang out there.
Comment on attachment 8417534 [details] [diff] [review]
added v8api files to patch

Cancelling obsolete needinfo.
Attachment #8417534 - Flags: feedback?(till)
SpiderNode development has been restarted on GitHub: https://github.com/mozilla/spidernode
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.