In my opinion the changes look great, at least the code diff has a good smell to it. (Some aspects, like build system decisions, I'm not qualified to judge.)

Why is the coroutine stack size increased from 128K to 256K on this platform? I'd like to add a comment telling people why.

Another weird thing: The change defining V8_NEEDS_BUFFER_ALLOCATOR to 1 in the middle of js_job.cc. That should presumably be done in some build config setting.

Another potential bug is that in src/build.mk the change only affects things if the compiler is GCC. What if it's another compiler?

Hi,

In the context_switching.cc change it looks like the new code saves registers below the stack pointer. All the other architectures decrement the stack pointer and then save the registers. Here are some diagrams of what the stacks look like after the artificial_stack_t constructor is called:

Existing architectures:

|   | <- unallocated memory (and maybe some allocated, but unaligned, memory)
+---+
|   | <- fake caller stack frame (of size min_frame)
+---+
|   |
|   | <- register save area (saved stack pointer points to top of register save area)
|   |
+---+ <- stack pointer

Power:

|   | <- unallocated memory (and maybe some allocated, but unaligned, memory)
+---+ <- stack pointer*
|   |
|   | <- register save area (saved stack pointer points to top of register save area)
|   |
+---+

 * sp += 24 removes the fake caller stack frame

Is there a reason power needs to behave differently? If so then it would be really good to add some comments.

Also I think there is a problem with removing the fake caller stack frame. If I understand the Power ELF V2 ABI correctly, then the min_frame constant should be 4 so that the initial_fun function can safely write LR to the LR save slot. As it stands I think initial_fun will in fact write LR into unallocated memory (beyond the top of the stack).

@srh @mundaym I appreciate your assessment and comments very much. I am working on the comments and suggestions. I will get back with appropriate code changes.

Implemented V8_NEEDS_BUFFER_ALLOCATOR related change. Pondering over the context switching discussion.

@srh About the potential bug in src/build.mk - present work used only GNU compiler hence added those GCC specific flags. Are there any specific compilers you would recommend for testing on ppc64le, apart from GNU's?

The change there adds general linking flags. I'd expect other compilers to need the same flags.

I don't expect you to test other compilers. If people see flags like that in a compiler-specific branch, they're likely to think the flags are GCC-only for some specific reason. And if Clang was used, you'd expect it to take the same flags.

Another bug in build.mk is that #if ! defined(__powerpc64__) isn't going to do anything there. It's a comment.

A better way might be to make pkg_link-flags in v8.sh output -licui18n, -licuuc, and -ldl. I believe the lines echoed out there get turned into linker parameters without any manipulation. This keeps the v8 library logic in the same place.

I have made change for V8_NEEDS_BUFFER_ALLOCATOR and the changes suggested in v8.sh, build.mk files.

Marked the review comments which are implemented.

@srh @mundaym All the review comments have been incorporated now. Could you please check and let me know if I missed out something? Also please let me know if you have additional queries, comments. Thanks!

Awesome work.

Ideally, if we set use_system_icu=1 when building v8, the configure script should test for ICU.

Otherwise the v8-related changes look okay to me. They aren't uglier than what we already have. Someday we'll get around to do #5112

@AtnNn Thanks for adding this PR to 2.5 milestones. Any estimate on timeframe when 2.5 can be expected? Also, any plans to create a new branch for 2.5?
Thanks!

@asowani The branch for 2.5 is next. We don't have a timeframe for 2.5. If I had to guess, I would say 4 to 12 months.

@AtnNn Thanks for letting me know about the estimated time frame for 2.5 release. I don't know much about how much effort it generally takes for new release preparation (may be creating a new branch?), so please pardon if I sound too eager. But is there any way we can have 2.5 release a bit earlier? If there is any help required, I am willing to assist. Thanks!

We're trying to get 2.4 ready right now. I'll try to get this patch into 2.4 if it doesn't delay the release.

@AtnNn - is v2.4.x the correct branch to check for merged patches for upcoming 2.4 release?

@asowani yes.

@AtnNn I have setup a Jenkins job to test v2.4.x branch whenever ppc64le code gets merged. At the moment I have disabled the job as I don't see those changes in the branch, but it can start the moment the changes get merged in the branch. I guess I will get an automated mail from github when this PR is merged, but just in case, I would highly appreciate if you could intimate here once the code is merged. Thanks!

@AtnNn -- what's your feeling on whether this will make it in to the 2.4 release or not? It still has the milestone, which gives me hope that it will. 😆

Just following up on the previous comment. Any outlook on whether this will be merging into 2.4 at this point? Thanks!

I'd say it's likely to make it into 2.4.

I've rebased and cleaned up the changes in this PR and put them in branches sam/24x/powerpc64 (based off v2.4.x) and sam/powerpc64 (based off next) in my repo at https://github.com/srh/rethinkdb/.

There are two things I'd still like to understand before putting this into 2.4.

First, what is the point of setting V8_NEEDS_BUFFER_ALLOCATOR?

Second, why does getifaddrs use alloca on powerpc64, but not on amd64? Is there an implementation I could look at in order to see this?

Edit: It looks like getifaddrs calls __netlink_request in glibc's sysdeps/unix/sysv/linux/ifaddrs.c, which then has

#ifdef PAGE_SIZE
  /* Help the compiler optimize out the malloc call if PAGE_SIZE
     is constant and smaller or equal to PTHREAD_STACK_MIN/4.  */
  const size_t buf_size = PAGE_SIZE;
#else
  const size_t buf_size = __getpagesize ();
#endif

after which, if buf_size <= 64K, it gets passed to alloca. And the page size could be 64KB.

I don't want to throw in these mysterious configuration options without putting an explanation in the source code.

There's a third thing. Why is v8 4.7 used for powerpc64? Is it necessary?

I've updated the branches referenced above to compute the coroutine stack size based on the page size value.

So there are now still two things I'd like to understand:

• why V8_BUFFER_ALLOCATOR?

• why v8 4.7?

@srh The selection of v8 4.7.x was already done in the script but was limited to Windows. Older versions of v8 are not ported to ppc64le and v8 4.7 has been ported to and tested on ppc64le, and is found to be working good on it. V8_NEEDS_BUFFER_ALLOCATOR is required at build time to have 4.7 version of v8.

@srh

Second, why does getifaddrs use alloca on powerpc64, but not on amd64? Is there an implementation I >could look at in order to see this?
This really depends on the pagezise configured on that arch and for powerpc64 it is passed as libc_use_alloca(65536) and it uses alloca().

@srh Any updates about code merge? Please let me know in case there are further queries. Thanks!

But what is it about ppc64le that makes v8 4.7 need V8_BUFFER_ALLOCATOR?

If I check out v8 4.7.80.23 and grep for V8_NEEDS_BUFFER_ALLOCATOR, I don't get any matches, and if I search on Google I don't get any results outside of this issue.

@srh I think this is with reference to commit e3de540

src/extproc/js_job.cc file has following code (around line 34):

#ifdef V8_NEEDS_BUFFER_ALLOCATOR
class array_buffer_allocator_t : public v8::ArrayBuffer::Allocator {
public:
...

I think the change is required to take care of this.

Oh, I'm a retard. Will merge this when I get back to a PC.

In next as of c300a18 and v2.4.x as of 4a71acf.

@srh Thanks very much for merging the changes!