Skip to content

Conversation

@Yuyupo
Copy link
Contributor

@Yuyupo Yuyupo commented Sep 4, 2018

IoT.js-DCO-1.0-Signed-off-by: Daniella Barsony bella@inf.u-szeged.hu

static const jerry_char_t *magic_string_items[] = {
#define MAGICSTR_EX_DEF(NAME, STRING) \
(const jerry_char_ptr_t) jerry_magic_string_ex_##NAME,
(const jerry_char_t *)jerry_magic_string_ex_##NAME,
Copy link
Contributor

Choose a reason for hiding this comment

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

Space is missing after the closing bracket.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually IoT.js style must be applied here (not JerryScript style), so the space is not needed.

if buildtype == 'release':
set_release_config_tizenrt()
# Fixme: EXTRA_LIBPATHS and EXTRA_LIB can be deleted
# when TizenRT uses jerry-ext
Copy link
Contributor

Choose a reason for hiding this comment

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

Use uppercase letters for fixme, and a dot is missing from the end of the sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, i missed that. Updated the PR accordingly.

'--profile=test/profiles/tizenrt.profile',
'EXTRA_LIBPATHS=-L' + DOCKER_IOTJS_PATH +
'/build/arm-tizenrt/' + buildtype + '/lib/',
'EXTRA_LIBS=-ljerry-ext'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need to provide the -L / -l option flags in the EXTRA_LIBPATHS and EXTRA_LIBS values?

Copy link
Contributor

@robertsipka robertsipka Sep 4, 2018

Choose a reason for hiding this comment

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

No need to answer, I see that you need to give it, but I was surprised at first.

Copy link
Contributor

@hs0225 hs0225 left a comment

Choose a reason for hiding this comment

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

.

Copy link
Contributor

@hs0225 hs0225 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@daeyeon daeyeon left a comment

Choose a reason for hiding this comment

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

@Yuyupo Thank you for the contribution!

To All: For debugger, should we take all extensions which could be duplicated with the one in IoT.js? Do we have any alternative? It seems almost harmless but different from the strategy I've understood so far. There was a consensus not to force use of jerry-extension in jerry community so it was separated as the lib. However, debugger seems a different kind.

@LaszloLango
Copy link
Contributor

@daeyeon usually the linker deals with the unused functions/features. @Yuyupo, @robertsipka, is this patch cause significant binary gain in any platform? IMHO the main goal of the jerry-ext should be to contain all of the non-standard features, like print builtin or debug server.

@zherczeg
Copy link
Member

zherczeg commented Sep 5, 2018

Yes, the design goal of jerry-ext is that the extensions are not depend on each other. So the linker can cut the unused parts. However this would be good to check in practice. I agree to check the binary size change.

@robertsipka
Copy link
Contributor

robertsipka commented Sep 5, 2018

After the libjerry update, the binary sizes with target and test specific profile builds - that contain all the modules that the tests require - are the following:

  • on STM32F4-Discovery : 247235 bytes.
  • on ARTIK 053 : 249596 bytes.
  • on Raspberry Pi 2 : 290130 bytes.

With the latest commit on IoT.js (40ee244) are the following:

  • on STM32F4-Discovery : 247731 bytes.
  • on ARTIK 053 : 252125 bytes.
  • on Raspberry Pi 2 : 292631 bytes.

Conclusion: The binary sizes will be decreased after this patch on these devices.

Updated: I noticed that this patch is not rebased with master. The previous commit is 354f28d on the PR, so there may be a difference in the comparison.
The binary sizes on 354f28d are the following:

  • on STM32F4-Discovery : 247468 bytes
  • on ARTIK 053: 253543 bytes
  • on Raspberry Pi 2 :292952 bytes

@zherczeg
Copy link
Member

zherczeg commented Sep 6, 2018

Nice! Thank you for the measurement.

@daeyeon
Copy link
Member

daeyeon commented Sep 6, 2018

The size will be more decreased if we can take only debugger from the extensions.

I'm fine that jerry-ext contains all the non-standard features. However, the more non-standard features merged in future will increase the size of jerry-ext, and that will affect the size of IoT.js.

I think that we need to suggest a way in jerryscript to selectively use the features we want. (Currently only debugger)

@LaszloLango
Copy link
Contributor

@Yuyupo jerryscript-project/jerryscript#2507 is merged. Could you update the jerry hash?

@Yuyupo
Copy link
Contributor Author

Yuyupo commented Sep 7, 2018

@LaszloLango Updated jerry to the latest.

@LaszloLango
Copy link
Contributor

@daeyeon We can open an issue in JerryScript to make the extensions configurable. It would be a good improvement in the engine. It might reduce the compile time too.

@daeyeon
Copy link
Member

daeyeon commented Sep 7, 2018

@LaszloLango Thank you for the opinion.

@Yuyupo I can't guess how much this update is urgent. Could you let me know whether waiting for the configuration is fine to you?

@LaszloLango
Copy link
Contributor

@daeyeon the update is needed for the VS Code Extension, because of the debugger related changes.

Copy link
Member

@daeyeon daeyeon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

@Yuyupo Please investigate the Travis failures.

IoT.js-DCO-1.0-Signed-off-by: Daniella Barsony bella@inf.u-szeged.hu
Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

Is the JerryScript hash correct? It looks quite old to me.

@LaszloLango LaszloLango dismissed their stale review September 7, 2018 10:38

Never mind. GitHub tricked me. The JerryScript hash is good. Sorry for the noise.

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

LGTM

@LaszloLango LaszloLango merged commit 7639ce6 into jerryscript-project:master Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants