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

Update nanojson #769

Closed
wants to merge 1 commit into from
Closed

Update nanojson #769

wants to merge 1 commit into from

Conversation

TobiGr
Copy link
Contributor

@TobiGr TobiGr commented Jan 2, 2022

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

Update our fork of nanojson. This includes a few dependency updates as well as smaller improvements from upstream.

@TobiGr TobiGr added the dependencies Pull requests that update a dependency file label Jan 2, 2022
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

2 points I have to discuss here:

  1. All tests are failing there seems to be something wrong
  2. What are the changes in the new "version"?

@FireMasterK
Copy link
Member

FireMasterK commented Sep 5, 2022

I just want to add that the current fork of nanojson uses significantly more CPU than upstream!

In Piped, I'm now using a personal fork since TeamPiped/Piped-Backend#357, the difference is MASSIVE!

I did this after seeing that nanojson was using a lot of CPU in the nextToken method. The NewPipe fork has changes, calling this method more times (for JS parsing?), which could be the cause of this.

I don't have a before screenshot, but here's the after (it was ~1800 seconds before, after 10 minutes of recording/profiling):
After:
image
nanojson is nowhere close to the top with this!

@opusforlife2
Copy link
Collaborator

@FireMasterK If the only difference between this version and your fork is upstream changes, could they not be added to this fork?

@FireMasterK
Copy link
Member

@FireMasterK If the only difference between this version and your fork is upstream changes, could they not be added to this fork?

I suppose they could, but I only tested it on YouTube (in Piped), I'm not sure if other services require the JS json extraction. In my fork, I have the upstream changes and have cherry-picked only this commit: TeamNewPipe/nanojson@d5da581

@Stypox
Copy link
Member

Stypox commented Nov 24, 2022

Closing in favor of #981. Thanks @FireMasterK for the heads-up!

@Stypox Stypox closed this Nov 24, 2022
@Stypox Stypox mentioned this pull request Nov 24, 2022
3 tasks
@TobiGr TobiGr deleted the dependencies/nanojson branch November 24, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants