WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 months ago

#14642 closed enhancement (fixed)

Support Facebook's HipHop

Reported by: ChenHui Owned by: nacin
Milestone: 3.1 Priority: normal
Severity: normal Version:
Component: Performance Keywords: has-patch
Focuses: Cc:

Description

Various changes to make WordPress compatible with HipHop, Facebook's
PHP-to-C++ transformer. With this patch, WordPress' codebase can be
tranformed to C++ and then compiled to binary that runs much faster
than the current PHP version.

Summary:

  • sanity checks for environmentals
  • string in HipHop doesn't have 'error' property
  • $user needs instantiation first before being used
  • always replace existing variables in registering new users ie. use EXTR_OVERWRITE in extract() by default

Attachments (1)

support_hphp.diff (3.4 KB) - added by ChenHui 4 years ago.
Patch to support HipHop

Download all attachments as: .zip

Change History (25)

ChenHui4 years ago

Patch to support HipHop

comment:1 ChenHui4 years ago

  • Component changed from General to Performance
  • Keywords has-patch added; hiphop removed

comment:2 ChenHui4 years ago

  • Cc usa.chen@… added

comment:3 follow-up: Denis-de-Bernardy4 years ago

I may be mistaking, but the extr_skip in users is there to protect $wpdb from being overwritten by a nutty user. or $userdata itself.

Why was hiphop chocking on users but not posts or terms?

comment:4 scribu4 years ago

  • Cc scribu added

comment:5 blepoxp4 years ago

  • Cc glenn@… added

comment:6 follow-up: hakre4 years ago

create_function ...

comment:8 in reply to: ↑ 3 ChenHui4 years ago

In HPHP, extract() doesn't overwrite some global variables (such as ID) which are needed by the insert_user function. PHP doesn't have such problem.

$wpdb could be removed from $userdata before extracting for safety.

Replying to Denis-de-Bernardy:

I may be mistaking, but the extr_skip in users is there to protect $wpdb from being overwritten by a nutty user. or $userdata itself.

Why was hiphop chocking on users but not posts or terms?

comment:9 in reply to: ↑ 6 ChenHui4 years ago

The create_function issue has been covered by another ticket submitted by Scott.

http://core.trac.wordpress.org/ticket/14424

Replying to hakre:

create_function ...

comment:10 in reply to: ↑ 7 ChenHui4 years ago

This one is rather old (Mar.11, 2010) and both WordPress and HipHop have evolved a lot since then.

This patch (combined with the create_function one) was proved to work with WordPress 3.0. Read my blog posts:

http://huichen.org/en/category/wordpress/

and announcement in HipHop mailing list:

http://groups.google.com/group/hiphop-php-dev/browse_thread/thread/8a1621c86f3aca2f/bc7c0c1e86d82b55

Replying to Denis-de-Bernardy:

http://talks.php.net/show/confoo10/15 (HipHop gotchas)

comment:11 nacin3 years ago

  • Milestone changed from Awaiting Review to 3.1
  • Owner set to nacin
  • Status changed from new to accepted

comment:12 nacin3 years ago

We use EXTR_SKIP in about 50 times in core. If HPHP doesn't support it, then it will take a lot more patching than that one instance.

comment:13 nacin3 years ago

(In [16377]) Properly check, initialize, or cast a number of variables. props ChenHui. see #14642.

comment:14 follow-up: nacin3 years ago

  • Keywords ongoing-project added

The other array casts seem to be deficiencies in the HPHP engine.

  • $qs will always be an array as it is used as the by reference argument for parse_str.
  • $r will always be an array, as it is either the result of get_object_vars(), $args within an is_array() check, or again parse_str.
  • $kayvees will always be an array as it is func_get_arg(0) within an is_array() check.

I'd rather not add casts just to add casts, when these should be reported upstream I think.

I'm just now seeing http://core.trac.wordpress.org/ticket/14642#comment:8 which explains that HPHP declines to extract the 'ID' key, hence why we need to kill EXTR_SKIP there specifically. I'm not convinced we should make our code more convoluted there for that one change. But, if it's the only thing to stop us from full support, then it could be considered.

In the meantime, we have more create_function calls to remove.

comment:15 in reply to: ↑ 14 westi3 years ago

Replying to nacin:

The other array casts seem to be deficiencies in the HPHP engine.

  • $qs will always be an array as it is used as the by reference argument for parse_str.
  • $r will always be an array, as it is either the result of get_object_vars(), $args within an is_array() check, or again parse_str.
  • $kayvees will always be an array as it is func_get_arg(0) within an is_array() check.

I'd rather not add casts just to add casts, when these should be reported upstream I think.

I'm just now seeing http://core.trac.wordpress.org/ticket/14642#comment:8 which explains that HPHP declines to extract the 'ID' key, hence why we need to kill EXTR_SKIP there specifically. I'm not convinced we should make our code more convoluted there for that one change. But, if it's the only thing to stop us from full support, then it could be considered.

This reads very much like a bug in HPHP that would affect a lot more than just us and could randomly affect other places extract is used too :-(

comment:16 nacin3 years ago

The change to wp-admin/includes/user.php, initializing $user as a WP_User object, is highly destructive. It causes existing passwords to be double-hashed on save. It may have other ramifications as well.

This was quite the fun bug hunt; thanks wpdavis for tipping me off.

comment:17 nacin3 years ago

(In [16443]) We don't want a populated WP_User object here. see #14642.

comment:18 jane3 years ago

We are past freeze and basically in beta. This doesn't seem like just a bug fix, and all committing developers should be focusing on bugs now, not enhancements. This should be punted to future release unless a lead developer believes it is a bug, not an enhancement.

comment:19 markjaquith3 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

Done with this for 3.1. Let's get back on it for 3.2-early.

comment:20 sbressler3 years ago

  • Keywords 3.4-early added; 3.2-early removed

comment:21 sbressler3 years ago

  • Cc sbressler@… added

comment:22 kawauso2 years ago

  • Cc kawauso added

comment:23 sheatsb2 years ago

  • Cc sheatsb added

comment:24 nacin3 months ago

  • Keywords ongoing-project 3.4-early removed
  • Milestone changed from Future Release to 3.1
  • Resolution set to fixed
  • Status changed from accepted to closed

Closing this as fixed against 3.1, where most of this activity occurred.

WordPress made a number of changes (in this ticket, before this ticket, after this ticket) to support HPHP. Over time, HPHP also got better. As of May 2012, I was told that the only remaining issue was wp-db.php, which defined the OBJECT constant as case-insensitive, which HPHP did not yet support. I explained (to said Facebook HPHP employee) that that's purely there for back compat (we could probably even remove it) and they could ignore this when building WP for HPHP. So we're compatible, that's that. Woo.

Note: See TracTickets for help on using tickets.